Skip to contentSkip to conversation

Linux RISC-V AIA Support

PATCH Superseded Anup Patel 3 participants 34 messages Oct 3, 2023 – Oct 23, 2023

Subsystems (6)

v10 of 16 Superseded Latest is v16 →

lore
Share & tools
lore

Anup Patel

author
#0 lore raw

v10 cover Linux RISC-V AIA Support

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

For an overview of the AIA specification, refer the AIA virtualization talk at KVM Forum 2022:
https://static.sched.com/hosted_files/kvmforum2022/a1/AIA_Virtualization_in_KVM_RISCV_final.pdf
https://www.youtube.com/watch?v=r071dL8Z0yo

To test this series, use QEMU v7.2 (or higher) and OpenSBI v1.2 (or higher).

These patches can also be found in the riscv_aia_v10 branch at:
https://github.com/avpatel/linux.git

Changes since v9:
- Rebased on Linux-6.6-rc4
- Use builtin_platform_driver() in PATCH5, PATCH9, and PATCH12

Changes since v8:
- Rebased on Linux-6.6-rc3
- Dropped PATCH2 of v8 series since we won't be requiring riscv_get_intc_hartid() based on Marc Z's comments on ACPI AIA support.
- Addressed Saravana's comments in PATCH3 of v8 series
- Update PATCH9 and PATCH13 of v8 series based on comments from Sunil

Changes since v7:
- Rebased on Linux-6.6-rc1
- Addressed comments on PATCH1 of v7 series and split it into two PATCHes
- Use DEFINE_SIMPLE_PROP() in PATCH2 of v7 series

Changes since v6:
- Rebased on Linux-6.5-rc4
- Updated PATCH2 to use IS_ENABLED(CONFIG_SPARC) instead of !IS_ENABLED(CONFIG_OF_IRQ)
- Added new PATCH4 to fix syscore registration in PLIC driver
- Update PATCH5 to convert PLIC driver into full-blown platform driver with a re-written probe function.

Changes since v5:
- Rebased on Linux-6.5-rc2
- Updated the overall series to ensure that only IPI, timer, and INTC drivers are probed very early whereas rest of the interrupt controllers (such as PLIC, APLIC, and IMISC) are probed as regular platform drivers.
- Renamed riscv_fw_parent_hartid() to riscv_get_intc_hartid()
- New PATCH1 to add fw_devlink support for msi-parent DT property
- New PATCH2 to ensure all INTC suppliers are initialized which in-turn fixes the probing issue for PLIC, APLIC and IMSIC as platform driver
- New PATCH3 to use platform driver probing for PLIC
- Re-structured the IMSIC driver into two separate drivers: early and platform. The IMSIC early driver (PATCH7) only initialized IMSIC state and provides IPIs whereas the IMSIC platform driver (PATCH8) is probed provides MSI domain for platform devices.
- Re-structure the APLIC platform driver into three separe sources: main, direct mode, and MSI mode.

Changes since v4:
- Rebased on Linux-6.5-rc1
- Added "Dependencies" in the APLIC bindings (PATCH6 in v4)
- Dropped the PATCH6 which was changing the IOMMU DMA domain APIs
- Dropped use of IOMMU DMA APIs in the IMSIC driver (PATCH4)

Changes since v3:
- Rebased on Linux-6.4-rc6
- Droped PATCH2 of v3 series instead we now set FWNODE_FLAG_BEST_EFFORT via IRQCHIP_DECLARE()
- Extend riscv_fw_parent_hartid() to support both DT and ACPI in PATCH1
- Extend iommu_dma_compose_msi_msg() instead of adding iommu_dma_select_msi() in PATCH6
- Addressed Conor's comments in PATCH3
- Addressed Conor's and Rob's comments in PATCH7

Changes since v2:
- Rebased on Linux-6.4-rc1
- Addressed Rob's comments on DT bindings patches 4 and 8.
- Addessed Marc's comments on IMSIC driver PATCH5
- Replaced use of OF apis in APLIC and IMSIC drivers with FWNODE apis this makes both drivers easily portable for ACPI support. This also removes unnecessary indirection from the APLIC and IMSIC drivers.
- PATCH1 is a new patch for portability with ACPI support
- PATCH2 is a new patch to fix probing in APLIC drivers for APLIC-only systems.
- PATCH7 is a new patch which addresses the IOMMU DMA domain issues pointed out by SiFive

Changes since v1:
- Rebased on Linux-6.2-rc2
- Addressed comments on IMSIC DT bindings for PATCH4
- Use raw_spin_lock_irqsave() on ids_lock for PATCH5
- Improved MMIO alignment checks in PATCH5 to allow MMIO regions with holes.
- Addressed comments on APLIC DT bindings for PATCH6
- Fixed warning splat in aplic_msi_write_msg() caused by zeroed MSI message in PATCH7
- Dropped DT property riscv,slow-ipi instead will have module parameter in future.

Anup Patel (15):
RISC-V: Don't fail in riscv_of_parent_hartid() for disabled HARTs
of: property: Add fw_devlink support for msi-parent
drivers: irqchip/riscv-intc: Mark all INTC nodes as initialized irqchip/sifive-plic: Fix syscore registration for multi-socket systems irqchip/sifive-plic: Convert PLIC driver into a platform driver irqchip/riscv-intc: Add support for RISC-V AIA
dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller
irqchip: Add RISC-V incoming MSI controller early driver irqchip/riscv-imsic: Add support for platform MSI irqdomain irqchip/riscv-imsic: Add support for PCI MSI irqdomain
dt-bindings: interrupt-controller: Add RISC-V advanced PLIC
irqchip: Add RISC-V advanced PLIC driver for direct-mode irqchip/riscv-aplic: Add support for MSI-mode
RISC-V: Select APLIC and IMSIC drivers
MAINTAINERS: Add entry for RISC-V AIA drivers

 .../interrupt-controller/riscv,aplic.yaml     | 172 ++++++
 .../interrupt-controller/riscv,imsics.yaml    | 172 ++++++
 MAINTAINERS                                   |  14 +
 arch/riscv/Kconfig                            |   2 +
 arch/riscv/kernel/cpu.c                       |  11 +-
 drivers/irqchip/Kconfig                       |  24 +
 drivers/irqchip/Makefile                      |   3 +
 drivers/irqchip/irq-riscv-aplic-direct.c      | 343 +++++++++++
 drivers/irqchip/irq-riscv-aplic-main.c        | 232 +++++++
 drivers/irqchip/irq-riscv-aplic-main.h        |  53 ++
 drivers/irqchip/irq-riscv-aplic-msi.c         | 285 +++++++++
 drivers/irqchip/irq-riscv-imsic-early.c       | 259 ++++++++
 drivers/irqchip/irq-riscv-imsic-platform.c    | 319 ++++++++++
 drivers/irqchip/irq-riscv-imsic-state.c       | 570 ++++++++++++++++++
 drivers/irqchip/irq-riscv-imsic-state.h       |  67 ++
 drivers/irqchip/irq-riscv-intc.c              |  44 +-
 drivers/irqchip/irq-sifive-plic.c             | 242 +++++---
 drivers/of/property.c                         |   2 +
 include/linux/irqchip/riscv-aplic.h           | 119 ++++
 include/linux/irqchip/riscv-imsic.h           |  86 +++
 20 files changed, 2915 insertions(+), 104 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
 create mode 100644 drivers/irqchip/irq-riscv-aplic-direct.c
 create mode 100644 drivers/irqchip/irq-riscv-aplic-main.c
 create mode 100644 drivers/irqchip/irq-riscv-aplic-main.h
 create mode 100644 drivers/irqchip/irq-riscv-aplic-msi.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-early.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-platform.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-state.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-state.h
 create mode 100644 include/linux/irqchip/riscv-aplic.h
 create mode 100644 include/linux/irqchip/riscv-imsic.h
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v10 1/15 RISC-V: Don't fail in riscv_of_parent_hartid() for disabled HARTs

The riscv_of_processor_hartid() used by riscv_of_parent_hartid() fails for HARTs disabled in the DT. This results in the following warning thrown by the RISC-V INTC driver for the E-core on SiFive boards:

[    0.000000] riscv-intc: unable to find hart id for /cpus/cpu@0/interrupt-controller

The riscv_of_parent_hartid() is only expected to read the hartid from the DT so we should directly call of_get_cpu_hwid() instead of calling riscv_of_processor_hartid().

Fixes ad635e723e17 ("riscv: cpu: Add 64bit hartid support on RV64")
---
 arch/riscv/kernel/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
arch/riscv/kernel/cpu.c +6 -5
index c17dacb1141c..157ace8b262c 100644
@@ -125,13 +125,14 @@ int __init riscv_early_of_processor_hartid(struct device_node *node, unsigned lo
  */
 int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid)
 {
-	int rc;
-
 	for (; node; node = node->parent) {
 		if (of_device_is_compatible(node, "riscv")) {
-			rc = riscv_of_processor_hartid(node, hartid);
-			if (!rc)
-				return 0;
+			*hartid = (unsigned long)of_get_cpu_hwid(node, 0);
+			if (*hartid == ~0UL) {
+				pr_warn("Found CPU without hart ID\n");
+				return -ENODEV;
+			}
+			return 0;
 		}
 	}
 
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH v10 2/15 of: property: Add fw_devlink support for msi-parent

In reply to Anup Patel

This allows fw_devlink to create device links between consumers of a MSI and the supplier of the MSI.

---
 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/of/property.c +2 -0
index cf8dacf3e3b8..afdaefbd03f6 100644
@@ -1267,6 +1267,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(panel, "panel", NULL)
+DEFINE_SIMPLE_PROP(msi_parent, "msi-parent", "#msi-cells")
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 
@@ -1356,6 +1357,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_leds, },
 	{ .parse_prop = parse_backlight, },
 	{ .parse_prop = parse_panel, },
+	{ .parse_prop = parse_msi_parent, },
 	{ .parse_prop = parse_gpio_compat, },
 	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },
Signature · 1 line
2.34.1
author
#3 lore raw

PATCH v10 3/15 drivers: irqchip/riscv-intc: Mark all INTC nodes as initialized

In reply to Anup Patel

The RISC-V INTC local interrupts are per-HART (or per-CPU) so we create INTC IRQ domain only for the INTC node belonging to the boot HART. This means only the boot HART INTC node will be marked as initialized and other INTC nodes won't be marked which results downstream interrupt controllers (such as PLIC, IMSIC and APLIC direct-mode) not being probed due to missing device suppliers.

To address this issue, we mark all INTC node for which we don't create IRQ domain as initialized.

---
 drivers/irqchip/irq-riscv-intc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/irqchip/irq-riscv-intc.c +9 -1
index 4adeee1bc391..e8d01b14ccdd 100644
@@ -155,8 +155,16 @@ static int __init riscv_intc_init(struct device_node *node,
 	 * for each INTC DT node. We only need to do INTC initialization
 	 * for the INTC DT node belonging to boot CPU (or boot HART).
 	 */
-	if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
+	if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) {
+		/*
+		 * The INTC nodes of each CPU are suppliers for downstream
+		 * interrupt controllers (such as PLIC, IMSIC and APLIC
+		 * direct-mode) so we should mark an INTC node as initialized
+		 * if we are not creating IRQ domain for it.
+		 */
+		fwnode_dev_initialized(of_fwnode_handle(node), true);
 		return 0;
+	}
 
 	return riscv_intc_init_common(of_node_to_fwnode(node));
 }
Signature · 1 line
2.34.1
author
#4 lore raw

PATCH v10 4/15 irqchip/sifive-plic: Fix syscore registration for multi-socket systems

In reply to Anup Patel

On multi-socket systems, we will have a separate PLIC in each socket so we should register syscore operation only once for multi-socket systems.

Fixes e80f0b6a2cf3 ("irqchip/irq-sifive-plic: Add syscore callbacks for hibernation")
---
 drivers/irqchip/irq-sifive-plic.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/irqchip/irq-sifive-plic.c +4 -3
index e1484905b7bd..5b7bc4fd9517 100644
@@ -532,17 +532,18 @@ static int __init __plic_init(struct device_node *node,
 	}
 
 	/*
-	 * We can have multiple PLIC instances so setup cpuhp state only
-	 * when context handler for current/boot CPU is present.
+	 * We can have multiple PLIC instances so setup cpuhp state
+	 * and register syscore operations only when context handler
+	 * for current/boot CPU is present.
 	 */
 	handler = this_cpu_ptr(&plic_handlers);
 	if (handler->present && !plic_cpuhp_setup_done) {
 		cpuhp_setup_state(CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
 				  "irqchip/sifive/plic:starting",
 				  plic_starting_cpu, plic_dying_cpu);
+		register_syscore_ops(&plic_irq_syscore_ops);
 		plic_cpuhp_setup_done = true;
 	}
-	register_syscore_ops(&plic_irq_syscore_ops);
 
 	pr_info("%pOFP: mapped %d interrupts with %d handlers for"
 		" %d contexts.\n", node, nr_irqs, nr_handlers, nr_contexts);
Signature · 1 line
2.34.1
author
#5 lore raw

PATCH v10 5/15 irqchip/sifive-plic: Convert PLIC driver into a platform driver

In reply to Anup Patel

The PLIC driver does not require very early initialization so let us convert it into a platform driver.

As part of the conversion, the PLIC probing undergoes the following changes:
1. Use dev_info(), dev_err() and dev_warn() instead of pr_info(), pr_err() and pr_warn()
2. Use devm_xyz() APIs wherever applicable
3. PLIC is now probed after CPUs are brought-up so we have to setup cpuhp state after context handler of all online CPUs are initialized otherwise we see crash on multi-socket systems

---
 drivers/irqchip/irq-sifive-plic.c | 239 ++++++++++++++++++------------
 1 file changed, 148 insertions(+), 91 deletions(-)
drivers/irqchip/irq-sifive-plic.c +148 -91
index 5b7bc4fd9517..c8f8a8cdcce1 100644
@@ -3,7 +3,6 @@
  * Copyright (C) 2017 SiFive
  * Copyright (C) 2018 Christoph Hellwig
  */
-#define pr_fmt(fmt) "plic: " fmt
 #include <linux/cpu.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -64,6 +63,7 @@
 #define PLIC_QUIRK_EDGE_INTERRUPT	0
 
 struct plic_priv {
+	struct device *dev;
 	struct cpumask lmask;
 	struct irq_domain *irqdomain;
 	void __iomem *regs;
@@ -85,7 +85,6 @@ struct plic_handler {
 	struct plic_priv	*priv;
 };
 static int plic_parent_irq __ro_after_init;
-static bool plic_cpuhp_setup_done __ro_after_init;
 static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
 
 static int plic_irq_set_type(struct irq_data *d, unsigned int type);
@@ -371,7 +370,8 @@ static void plic_handle_irq(struct irq_desc *desc)
 		int err = generic_handle_domain_irq(handler->priv->irqdomain,
 						    hwirq);
 		if (unlikely(err))
-			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
+			dev_warn_ratelimited(handler->priv->dev,
+					"can't find mapping for hwirq %lu\n",
 					hwirq);
 	}
 
@@ -406,57 +406,126 @@ static int plic_starting_cpu(unsigned int cpu)
 	return 0;
 }
 
-static int __init __plic_init(struct device_node *node,
-			      struct device_node *parent,
-			      unsigned long plic_quirks)
+static const struct of_device_id plic_match[] = {
+	{ .compatible = "sifive,plic-1.0.0" },
+	{ .compatible = "riscv,plic0" },
+	{ .compatible = "andestech,nceplic100",
+	  .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
+	{ .compatible = "thead,c900-plic",
+	  .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
+	{}
+};
+
+static int plic_parse_nr_irqs_and_contexts(struct platform_device *pdev,
+					   u32 *nr_irqs, u32 *nr_contexts)
 {
-	int error = 0, nr_contexts, nr_handlers = 0, i;
-	u32 nr_irqs;
-	struct plic_priv *priv;
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	/*
+	 * Currently, only OF fwnode is supported so extend this
+	 * function for ACPI support.
+	 */
+	if (!is_of_node(dev->fwnode))
+		return -EINVAL;
+
+	rc = of_property_read_u32(to_of_node(dev->fwnode),
+				  "riscv,ndev", nr_irqs);
+	if (rc) {
+		dev_err(dev, "riscv,ndev property not available\n");
+		return rc;
+	}
+
+	*nr_contexts = of_irq_count(to_of_node(dev->fwnode));
+	if (WARN_ON(!(*nr_contexts))) {
+		dev_err(dev, "no PLIC context available\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int plic_parse_context_parent_hwirq(struct platform_device *pdev,
+					   u32 context, u32 *parent_hwirq,
+					   unsigned long *parent_hartid)
+{
+	struct device *dev = &pdev->dev;
+	struct of_phandle_args parent;
+	int rc;
+
+	/*
+	 * Currently, only OF fwnode is supported so extend this
+	 * function for ACPI support.
+	 */
+	if (!is_of_node(dev->fwnode))
+		return -EINVAL;
+
+	rc = of_irq_parse_one(to_of_node(dev->fwnode), context, &parent);
+	if (rc)
+		return rc;
+
+	rc = riscv_of_parent_hartid(parent.np, parent_hartid);
+	if (rc)
+		return rc;
+
+	*parent_hwirq = parent.args[0];
+	return 0;
+}
+
+static int plic_probe(struct platform_device *pdev)
+{
+	int rc, nr_contexts, nr_handlers = 0, i, cpu;
+	unsigned long plic_quirks = 0, hartid;
+	struct device *dev = &pdev->dev;
 	struct plic_handler *handler;
-	unsigned int cpu;
+	u32 nr_irqs, parent_hwirq;
+	struct irq_domain *domain;
+	struct plic_priv *priv;
+	irq_hw_number_t hwirq;
+	struct resource *res;
+	bool cpuhp_setup;
+
+	if (is_of_node(dev->fwnode)) {
+		const struct of_device_id *id;
+
+		id = of_match_node(plic_match, to_of_node(dev->fwnode));
+		if (id)
+			plic_quirks = (unsigned long)id->data;
+	}
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-
+	priv->dev = dev;
 	priv->plic_quirks = plic_quirks;
 
-	priv->regs = of_iomap(node, 0);
-	if (WARN_ON(!priv->regs)) {
-		error = -EIO;
-		goto out_free_priv;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "failed to get MMIO resource\n");
+		return -EINVAL;
+	}
+	priv->regs = devm_ioremap(dev, res->start, resource_size(res));
+	if (!priv->regs) {
+		dev_err(dev, "failed map MMIO registers\n");
+		return -EIO;
 	}
 
-	error = -EINVAL;
-	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
-	if (WARN_ON(!nr_irqs))
-		goto out_iounmap;
-
+	rc = plic_parse_nr_irqs_and_contexts(pdev, &nr_irqs, &nr_contexts);
+	if (rc) {
+		dev_err(dev, "failed to parse irqs and contexts\n");
+		return rc;
+	}
 	priv->nr_irqs = nr_irqs;
 
-	priv->prio_save = bitmap_alloc(nr_irqs, GFP_KERNEL);
+	priv->prio_save = devm_bitmap_zalloc(dev, nr_irqs, GFP_KERNEL);
 	if (!priv->prio_save)
-		goto out_free_priority_reg;
-
-	nr_contexts = of_irq_count(node);
-	if (WARN_ON(!nr_contexts))
-		goto out_free_priority_reg;
-
-	error = -ENOMEM;
-	priv->irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
-			&plic_irqdomain_ops, priv);
-	if (WARN_ON(!priv->irqdomain))
-		goto out_free_priority_reg;
+		return -ENOMEM;
 
 	for (i = 0; i < nr_contexts; i++) {
-		struct of_phandle_args parent;
-		irq_hw_number_t hwirq;
-		int cpu;
-		unsigned long hartid;
-
-		if (of_irq_parse_one(node, i, &parent)) {
-			pr_err("failed to parse parent for context %d.\n", i);
+		rc = plic_parse_context_parent_hwirq(pdev, i,
+						     &parent_hwirq, &hartid);
+		if (rc) {
+			dev_warn(dev, "hwirq for context%d not found\n", i);
 			continue;
 		}
 
@@ -464,7 +533,7 @@ static int __init __plic_init(struct device_node *node,
 		 * Skip contexts other than external interrupts for our
 		 * privilege level.
 		 */
-		if (parent.args[0] != RV_IRQ_EXT) {
+		if (parent_hwirq != RV_IRQ_EXT) {
 			/* Disable S-mode enable bits if running in M-mode. */
 			if (IS_ENABLED(CONFIG_RISCV_M_MODE)) {
 				void __iomem *enable_base = priv->regs +
@@ -477,21 +546,17 @@ static int __init __plic_init(struct device_node *node,
 			continue;
 		}
 
-		error = riscv_of_parent_hartid(parent.np, &hartid);
-		if (error < 0) {
-			pr_warn("failed to parse hart ID for context %d.\n", i);
-			continue;
-		}
-
 		cpu = riscv_hartid_to_cpuid(hartid);
 		if (cpu < 0) {
-			pr_warn("Invalid cpuid for context %d\n", i);
+			dev_warn(dev, "Invalid cpuid for context %d\n", i);
 			continue;
 		}
 
 		/* Find parent domain and register chained handler */
-		if (!plic_parent_irq && irq_find_host(parent.np)) {
-			plic_parent_irq = irq_of_parse_and_map(node, i);
+		domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(),
+						  DOMAIN_BUS_ANY);
+		if (!plic_parent_irq && domain) {
+			plic_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
 			if (plic_parent_irq)
 				irq_set_chained_handler(plic_parent_irq,
 							plic_handle_irq);
@@ -504,7 +569,7 @@ static int __init __plic_init(struct device_node *node,
 		 */
 		handler = per_cpu_ptr(&plic_handlers, cpu);
 		if (handler->present) {
-			pr_warn("handler already present for context %d.\n", i);
+			dev_warn(dev, "handler already present for context%d.\n", i);
 			plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD);
 			goto done;
 		}
@@ -518,10 +583,13 @@ static int __init __plic_init(struct device_node *node,
 			i * CONTEXT_ENABLE_SIZE;
 		handler->priv = priv;
 
-		handler->enable_save =  kcalloc(DIV_ROUND_UP(nr_irqs, 32),
-						sizeof(*handler->enable_save), GFP_KERNEL);
+		handler->enable_save =  devm_kcalloc(dev,
+						DIV_ROUND_UP(nr_irqs, 32),
+						sizeof(*handler->enable_save),
+						GFP_KERNEL);
 		if (!handler->enable_save)
-			goto out_free_enable_reg;
+			return -ENOMEM;
+
 done:
 		for (hwirq = 1; hwirq <= nr_irqs; hwirq++) {
 			plic_toggle(handler, hwirq, 0);
@@ -531,52 +599,41 @@ static int __init __plic_init(struct device_node *node,
 		nr_handlers++;
 	}
 
+	priv->irqdomain = irq_domain_create_linear(dev->fwnode, nr_irqs + 1,
+						   &plic_irqdomain_ops, priv);
+	if (WARN_ON(!priv->irqdomain))
+		return -ENOMEM;
+
 	/*
 	 * We can have multiple PLIC instances so setup cpuhp state
-	 * and register syscore operations only when context handler
-	 * for current/boot CPU is present.
+	 * and register syscore operations only after context handlers
+	 * of all online CPUs are initialized.
 	 */
-	handler = this_cpu_ptr(&plic_handlers);
-	if (handler->present && !plic_cpuhp_setup_done) {
+	cpuhp_setup = true;
+	for_each_online_cpu(cpu) {
+		handler = per_cpu_ptr(&plic_handlers, cpu);
+		if (!handler->present) {
+			cpuhp_setup = false;
+			break;
+		}
+	}
+	if (cpuhp_setup) {
 		cpuhp_setup_state(CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
 				  "irqchip/sifive/plic:starting",
 				  plic_starting_cpu, plic_dying_cpu);
 		register_syscore_ops(&plic_irq_syscore_ops);
-		plic_cpuhp_setup_done = true;
 	}
 
-	pr_info("%pOFP: mapped %d interrupts with %d handlers for"
-		" %d contexts.\n", node, nr_irqs, nr_handlers, nr_contexts);
+	dev_info(dev, "mapped %d interrupts with %d handlers for"
+		" %d contexts.\n", nr_irqs, nr_handlers, nr_contexts);
 	return 0;
-
-out_free_enable_reg:
-	for_each_cpu(cpu, cpu_present_mask) {
-		handler = per_cpu_ptr(&plic_handlers, cpu);
-		kfree(handler->enable_save);
-	}
-out_free_priority_reg:
-	kfree(priv->prio_save);
-out_iounmap:
-	iounmap(priv->regs);
-out_free_priv:
-	kfree(priv);
-	return error;
 }
 
-static int __init plic_init(struct device_node *node,
-			    struct device_node *parent)
-{
-	return __plic_init(node, parent, 0);
-}
-
-IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
-IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
-
-static int __init plic_edge_init(struct device_node *node,
-				 struct device_node *parent)
-{
-	return __plic_init(node, parent, BIT(PLIC_QUIRK_EDGE_INTERRUPT));
-}
-
-IRQCHIP_DECLARE(andestech_nceplic100, "andestech,nceplic100", plic_edge_init);
-IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_edge_init);
+static struct platform_driver plic_driver = {
+	.driver = {
+		.name		= "riscv-plic",
+		.of_match_table	= plic_match,
+	},
+	.probe = plic_probe,
+};
+builtin_platform_driver(plic_driver);
Signature · 1 line
2.34.1
author
#6 lore raw

PATCH v10 6/15 irqchip/riscv-intc: Add support for RISC-V AIA

In reply to Anup Patel

The RISC-V advanced interrupt architecture (AIA) extends the per-HART local interrupts in following ways:
1. Minimum 64 local interrupts for both RV32 and RV64
2. Ability to process multiple pending local interrupts in same interrupt handler
3. Priority configuration for each local interrupts
4. Special CSRs to configure/access the per-HART MSI controller

We add support for #1 and #2 described above in the RISC-V intc driver.

---
 drivers/irqchip/irq-riscv-intc.c | 34 ++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)
drivers/irqchip/irq-riscv-intc.c +28 -6
index e8d01b14ccdd..bab536bbaf2c 100644
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/smp.h>
+#include <asm/hwcap.h>
 
 static struct irq_domain *intc_domain;
 
@@ -30,6 +31,15 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
 	generic_handle_domain_irq(intc_domain, cause);
 }
 
+static asmlinkage void riscv_intc_aia_irq(struct pt_regs *regs)
+{
+	unsigned long topi;
+
+	while ((topi = csr_read(CSR_TOPI)))
+		generic_handle_domain_irq(intc_domain,
+					  topi >> TOPI_IID_SHIFT);
+}
+
 /*
  * On RISC-V systems local interrupts are masked or unmasked by writing
  * the SIE (Supervisor Interrupt Enable) CSR.  As CSRs can only be written
@@ -39,12 +49,18 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
 
 static void riscv_intc_irq_mask(struct irq_data *d)
 {
-	csr_clear(CSR_IE, BIT(d->hwirq));
+	if (IS_ENABLED(CONFIG_32BIT) && d->hwirq >= BITS_PER_LONG)
+		csr_clear(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));
+	else
+		csr_clear(CSR_IE, BIT(d->hwirq));
 }
 
 static void riscv_intc_irq_unmask(struct irq_data *d)
 {
-	csr_set(CSR_IE, BIT(d->hwirq));
+	if (IS_ENABLED(CONFIG_32BIT) && d->hwirq >= BITS_PER_LONG)
+		csr_set(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));
+	else
+		csr_set(CSR_IE, BIT(d->hwirq));
 }
 
 static void riscv_intc_irq_eoi(struct irq_data *d)
@@ -115,16 +131,20 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
 
 static int __init riscv_intc_init_common(struct fwnode_handle *fn)
 {
-	int rc;
+	int rc, nr_irqs = riscv_isa_extension_available(NULL, SxAIA) ?
+			  64 : BITS_PER_LONG;
 
-	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
+	intc_domain = irq_domain_create_linear(fn, nr_irqs,
 					       &riscv_intc_domain_ops, NULL);
 	if (!intc_domain) {
 		pr_err("unable to add IRQ domain\n");
 		return -ENXIO;
 	}
 
-	rc = set_handle_irq(&riscv_intc_irq);
+	if (riscv_isa_extension_available(NULL, SxAIA))
+		rc = set_handle_irq(&riscv_intc_aia_irq);
+	else
+		rc = set_handle_irq(&riscv_intc_irq);
 	if (rc) {
 		pr_err("failed to set irq handler\n");
 		return rc;
@@ -132,7 +152,9 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
 
 	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
 
-	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
+	pr_info("%d local interrupts mapped%s\n",
+		nr_irqs, riscv_isa_extension_available(NULL, SxAIA) ?
+			 " using AIA" : "");
 
 	return 0;
 }
Signature · 1 line
2.34.1
author
#7 lore raw

PATCH v10 7/15 dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

In reply to Anup Patel

We add DT bindings document for the RISC-V incoming MSI controller (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA) specification.

---
 .../interrupt-controller/riscv,imsics.yaml    | 172 ++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml new file +172 -0
new file mode 100644
index 000000000000..84976f17a4a1
@@ -0,0 +1,172 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Incoming MSI Controller (IMSIC)
+
+maintainers:
+  - Anup Patel <[email protected]>
+
+description: |
+  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
+  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
+  AIA specification can be found at https://github.com/riscv/riscv-aia.
+
+  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
+  for each privilege level (machine or supervisor). The configuration of
+  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
+  space to receive MSIs from devices. Each IMSIC interrupt file supports a
+  fixed number of interrupt identities (to distinguish MSIs from devices)
+  which is same for given privilege level across CPUs (or HARTs).
+
+  The device tree of a RISC-V platform will have one IMSIC device tree node
+  for each privilege level (machine or supervisor) which collectively describe
+  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
+
+  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
+  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
+  group is a set of IMSIC interrupt files co-located in MMIO space and we can
+  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
+  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
+  privilege level (machine or supervisor) encodes group index, HART index,
+  and guest index (shown below).
+
+  XLEN-1            > (HART Index MSB)                  12    0
+  |                  |                                  |     |
+  -------------------------------------------------------------
+  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
+  -------------------------------------------------------------
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qemu,imsics
+      - const: riscv,imsics
+
+  reg:
+    minItems: 1
+    maxItems: 16384
+    description:
+      Base address of each IMSIC group.
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 0
+
+  msi-controller: true
+
+  "#msi-cells":
+    const: 0
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 16384
+    description:
+      This property represents the set of CPUs (or HARTs) for which given
+      device tree node describes the IMSIC interrupt files. Each node pointed
+      to should be a riscv,cpu-intc node, which has a CPU node (i.e. RISC-V
+      HART) as parent.
+
+  riscv,num-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities supported by IMSIC interrupt file.
+
+  riscv,num-guest-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities are supported by IMSIC guest interrupt
+      file. When not specified it is assumed to be same as specified by the
+      riscv,num-ids property.
+
+  riscv,guest-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of guest index bits in the MSI target address.
+
+  riscv,hart-index-bits:
+    minimum: 0
+    maximum: 15
+    description:
+      Number of HART index bits in the MSI target address. When not
+      specified it is calculated based on the interrupts-extended property.
+
+  riscv,group-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of group index bits in the MSI target address.
+
+  riscv,group-index-shift:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 55
+    default: 24
+    description:
+      The least significant bit position of the group index bits in the
+      MSI target address.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - msi-controller
+  - "#msi-cells"
+  - interrupts-extended
+  - riscv,num-ids
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example 1 (Machine-level IMSIC files with just one group):
+
+    interrupt-controller@24000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 11>,
+                            <&cpu2_intc 11>,
+                            <&cpu3_intc 11>,
+                            <&cpu4_intc 11>;
+      reg = <0x28000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+    };
+
+  - |
+    // Example 2 (Supervisor-level IMSIC files with two groups):
+
+    interrupt-controller@28000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 9>,
+                            <&cpu2_intc 9>,
+                            <&cpu3_intc 9>,
+                            <&cpu4_intc 9>;
+      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
+            <0x29000000 0x2000>; /* Group1 IMSICs */
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+      riscv,group-index-bits = <1>;
+      riscv,group-index-shift = <24>;
+    };
+...
Signature · 1 line
2.34.1
#8 lore raw

Re: [PATCH v10 07/15] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

Hey,

Quoted 6 lines · Anup Patel

We add DT bindings document for the RISC-V incoming MSI controller (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA) specification.

Just FYI, since they'll reply to this themselves, but some of the Microchip folks have run into problems with sparse hart indexes while trying to use the imsic binding to describe some configurations they have. I think there were also so problems with how to describe to a linux guest which file to use, when the first hart available to the guest does not use the first file. They'll do a better job of describing their problems than I will, so I shall leave it to them!

Cheers,
Conor.

Quoted 187 lines
---
 .../interrupt-controller/riscv,imsics.yaml    | 172 ++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml new file +172 -0
new file mode 100644
index 000000000000..84976f17a4a1
@@ -0,0 +1,172 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Incoming MSI Controller (IMSIC)
+
+maintainers:
+  - Anup Patel <[email protected]>
+
+description: |
+  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
+  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
+  AIA specification can be found at https://github.com/riscv/riscv-aia.
+
+  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
+  for each privilege level (machine or supervisor). The configuration of
+  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
+  space to receive MSIs from devices. Each IMSIC interrupt file supports a
+  fixed number of interrupt identities (to distinguish MSIs from devices)
+  which is same for given privilege level across CPUs (or HARTs).
+
+  The device tree of a RISC-V platform will have one IMSIC device tree node
+  for each privilege level (machine or supervisor) which collectively describe
+  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
+
+  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
+  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
+  group is a set of IMSIC interrupt files co-located in MMIO space and we can
+  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
+  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
+  privilege level (machine or supervisor) encodes group index, HART index,
+  and guest index (shown below).
+
+  XLEN-1            > (HART Index MSB)                  12    0
+  |                  |                                  |     |
+  -------------------------------------------------------------
+  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
+  -------------------------------------------------------------
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qemu,imsics
+      - const: riscv,imsics
+
+  reg:
+    minItems: 1
+    maxItems: 16384
+    description:
+      Base address of each IMSIC group.
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 0
+
+  msi-controller: true
+
+  "#msi-cells":
+    const: 0
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 16384
+    description:
+      This property represents the set of CPUs (or HARTs) for which given
+      device tree node describes the IMSIC interrupt files. Each node pointed
+      to should be a riscv,cpu-intc node, which has a CPU node (i.e. RISC-V
+      HART) as parent.
+
+  riscv,num-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities supported by IMSIC interrupt file.
+
+  riscv,num-guest-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities are supported by IMSIC guest interrupt
+      file. When not specified it is assumed to be same as specified by the
+      riscv,num-ids property.
+
+  riscv,guest-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of guest index bits in the MSI target address.
+
+  riscv,hart-index-bits:
+    minimum: 0
+    maximum: 15
+    description:
+      Number of HART index bits in the MSI target address. When not
+      specified it is calculated based on the interrupts-extended property.
+
+  riscv,group-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of group index bits in the MSI target address.
+
+  riscv,group-index-shift:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 55
+    default: 24
+    description:
+      The least significant bit position of the group index bits in the
+      MSI target address.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - msi-controller
+  - "#msi-cells"
+  - interrupts-extended
+  - riscv,num-ids
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example 1 (Machine-level IMSIC files with just one group):
+
+    interrupt-controller@24000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 11>,
+                            <&cpu2_intc 11>,
+                            <&cpu3_intc 11>,
+                            <&cpu4_intc 11>;
+      reg = <0x28000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+    };
+
+  - |
+    // Example 2 (Supervisor-level IMSIC files with two groups):
+
+    interrupt-controller@28000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 9>,
+                            <&cpu2_intc 9>,
+                            <&cpu3_intc 9>,
+                            <&cpu4_intc 9>;
+      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
+            <0x29000000 0x2000>; /* Group1 IMSICs */
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+      riscv,group-index-bits = <1>;
+      riscv,group-index-shift = <24>;
+    };
+...
Signature · 1 line
2.34.1
author
#9 lore raw

Re: [PATCH v10 07/15] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

Quoted 11 lines · Conor Dooley

Hey,

Quoted 6 lines · Anup Patel

We add DT bindings document for the RISC-V incoming MSI controller (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA) specification.

Quoted 7 lines

Just FYI, since they'll reply to this themselves, but some of the Microchip folks have run into problems with sparse hart indexes while trying to use the imsic binding to describe some configurations they have. I think there were also so problems with how to describe to a linux guest which file to use, when the first hart available to the guest does not use the first file. They'll do a better job of describing their problems than I will, so I shall leave it to them!

Quoting AIA spec:
"For the purpose of locating the memory pages of interrupt files in the address space, assume each hart (or each hart within a group) has a unique hart number that may or may not be related to the unique hart identifiers (“hart IDs”) that the RISC-V Privileged Architecture assigns to harts."

It is very easy to get confused between the AIA "hart index" and "hart IDs" defined by the RISC-V Privileged specification but these are two very different things. The AIA "hart index" over here is the bits in the address of an IMSIC file.

This DT binding follows the IMSIC file arrangement in the address space as defined by the section "3.6 Arrangement of the memory regions of multiple interrupt files" of the AIA specification. This arrangement is MANDATORY for platforms having both APLIC and IMSIC because in MSI-mode the APLIC generates target MSI address based the IMSIC file arrangement described in the section "3.6 Arrangement of the memory regions of multiple interrupt files". In fact, this also applies to virtual platforms created by hypervisors (KVM, Xen, ...)

Regards,
Anup

Quoted 191 lines

Cheers,
Conor.

Quoted 187 lines
---
 .../interrupt-controller/riscv,imsics.yaml    | 172 ++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml new file +172 -1
new file mode 100644
index 000000000000..84976f17a4a1
@@ -0,0 +1,172 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Incoming MSI Controller (IMSIC)
+
+maintainers:
+  - Anup Patel <[email protected]>
+
+description: |
+  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
+  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
+  AIA specification can be found at https://github.com/riscv/riscv-aia.
+
+  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
+  for each privilege level (machine or supervisor). The configuration of
+  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
+  space to receive MSIs from devices. Each IMSIC interrupt file supports a
+  fixed number of interrupt identities (to distinguish MSIs from devices)
+  which is same for given privilege level across CPUs (or HARTs).
+
+  The device tree of a RISC-V platform will have one IMSIC device tree node
+  for each privilege level (machine or supervisor) which collectively describe
+  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
+
+  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
+  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
+  group is a set of IMSIC interrupt files co-located in MMIO space and we can
+  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
+  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
+  privilege level (machine or supervisor) encodes group index, HART index,
+  and guest index (shown below).
+
+  XLEN-1            > (HART Index MSB)                  12    0
+  |                  |                                  |     |
+  -------------------------------------------------------------
+  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
+  -------------------------------------------------------------
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qemu,imsics
+      - const: riscv,imsics
+
+  reg:
+    minItems: 1
+    maxItems: 16384
+    description:
+      Base address of each IMSIC group.
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 0
+
+  msi-controller: true
+
+  "#msi-cells":
+    const: 0
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 16384
+    description:
+      This property represents the set of CPUs (or HARTs) for which given
+      device tree node describes the IMSIC interrupt files. Each node pointed
+      to should be a riscv,cpu-intc node, which has a CPU node (i.e. RISC-V
+      HART) as parent.
+
+  riscv,num-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities supported by IMSIC interrupt file.
+
+  riscv,num-guest-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 63
+    maximum: 2047
+    description:
+      Number of interrupt identities are supported by IMSIC guest interrupt
+      file. When not specified it is assumed to be same as specified by the
+      riscv,num-ids property.
+
+  riscv,guest-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of guest index bits in the MSI target address.
+
+  riscv,hart-index-bits:
+    minimum: 0
+    maximum: 15
+    description:
+      Number of HART index bits in the MSI target address. When not
+      specified it is calculated based on the interrupts-extended property.
+
+  riscv,group-index-bits:
+    minimum: 0
+    maximum: 7
+    default: 0
+    description:
+      Number of group index bits in the MSI target address.
+
+  riscv,group-index-shift:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 55
+    default: 24
+    description:
+      The least significant bit position of the group index bits in the
+      MSI target address.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - msi-controller
+  - "#msi-cells"
+  - interrupts-extended
+  - riscv,num-ids
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example 1 (Machine-level IMSIC files with just one group):
+
+    interrupt-controller@24000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 11>,
+                            <&cpu2_intc 11>,
+                            <&cpu3_intc 11>,
+                            <&cpu4_intc 11>;
+      reg = <0x28000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+    };
+
+  - |
+    // Example 2 (Supervisor-level IMSIC files with two groups):
+
+    interrupt-controller@28000000 {
+      compatible = "qemu,imsics", "riscv,imsics";
+      interrupts-extended = <&cpu1_intc 9>,
+                            <&cpu2_intc 9>,
+                            <&cpu3_intc 9>,
+                            <&cpu4_intc 9>;
+      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
+            <0x29000000 0x2000>; /* Group1 IMSICs */
+      interrupt-controller;
+      #interrupt-cells = <0>;
+      msi-controller;
+      #msi-cells = <0>;
+      riscv,num-ids = <127>;
+      riscv,group-index-bits = <1>;
+      riscv,group-index-shift = <24>;
+    };
+...
--

2.34.1

#10 lore raw

Re: [PATCH v10 07/15] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

Hey Anup,

Quoted 29 lines · Anup Patel
Quoted 8 lines · Conor Dooley
Quoted 6 lines · Anup Patel

We add DT bindings document for the RISC-V incoming MSI controller (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA) specification.

Quoted 7 lines

Just FYI, since they'll reply to this themselves, but some of the Microchip folks have run into problems with sparse hart indexes while trying to use the imsic binding to describe some configurations they have. I think there were also so problems with how to describe to a linux guest which file to use, when the first hart available to the guest does not use the first file. They'll do a better job of describing their problems than I will, so I shall leave it to them!

Quoting AIA spec:
"For the purpose of locating the memory pages of interrupt files in the address space, assume each hart (or each hart within a group) has a unique hart number that may or may not be related to the unique hart identifiers (“hart IDs”) that the RISC-V Privileged Architecture assigns to harts."

It is very easy to get confused between the AIA "hart index" and "hart IDs" defined by the RISC-V Privileged specification but these are two very different things. The AIA "hart index" over here is the bits in the address of an IMSIC file.

Quoted 9 lines

This DT binding follows the IMSIC file arrangement in the address space as defined by the section "3.6 Arrangement of the memory regions of multiple interrupt files" of the AIA specification. This arrangement is MANDATORY for platforms having both APLIC and IMSIC because in MSI-mode the APLIC generates target MSI address based the IMSIC file arrangement described in the section "3.6 Arrangement of the memory regions of multiple interrupt files". In fact, this also applies to virtual platforms created by hypervisors (KVM, Xen, ...)

Thanks for pointing this out - I'll pass it on and hopefully it is helpful to them. If not, I expect that you'll hear :)

Cheers,
Conor.

author
#11 lore raw

PATCH v10 8/15 irqchip: Add RISC-V incoming MSI controller early driver

In reply to Anup Patel

The RISC-V advanced interrupt architecture (AIA) specification defines a new MSI controller called incoming message signalled interrupt controller (IMSIC) which manages MSI on per-HART (or per-CPU) basis. It also supports IPIs as software injected MSIs. (For more details refer https://github.com/riscv/riscv-aia)

Let us add an early irqchip driver for RISC-V IMSIC which sets up the IMSIC state and provide IPIs.

---
 drivers/irqchip/Kconfig                 |   6 +
 drivers/irqchip/Makefile                |   1 +
 drivers/irqchip/irq-riscv-imsic-early.c | 259 +++++++++++
 drivers/irqchip/irq-riscv-imsic-state.c | 570 ++++++++++++++++++++++++
 drivers/irqchip/irq-riscv-imsic-state.h |  66 +++
 include/linux/irqchip/riscv-imsic.h     |  86 ++++
 6 files changed, 988 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-imsic-early.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-state.c
 create mode 100644 drivers/irqchip/irq-riscv-imsic-state.h
 create mode 100644 include/linux/irqchip/riscv-imsic.h
drivers/irqchip/Kconfig +6 -0
index f7149d0f3d45..bdd80716114d 100644
@@ -546,6 +546,12 @@ config SIFIVE_PLIC
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
+config RISCV_IMSIC
+	bool
+	depends on RISCV
+	select IRQ_DOMAIN_HIERARCHY
+	select GENERIC_MSI_IRQ
+
 config EXYNOS_IRQ_COMBINER
 	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
 	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
drivers/irqchip/Makefile +1 -0
index ffd945fe71aa..d714724387ce 100644
@@ -95,6 +95,7 @@ obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
+obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
drivers/irqchip/irq-riscv-imsic-early.c new file +259 -0

259 lines not on this page · raw message

drivers/irqchip/irq-riscv-imsic-state.c new file +570 -0

570 lines not on this page · raw message

drivers/irqchip/irq-riscv-imsic-state.h new file +66 -0

66 lines not on this page · raw message

include/linux/irqchip/riscv-imsic.h new file +86 -0

86 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#12 lore raw

PATCH v10 9/15 irqchip/riscv-imsic: Add support for platform MSI irqdomain

In reply to Anup Patel

The Linux platform MSI support requires a platform MSI irqdomain so let us add a platform irqchip driver for RISC-V IMSIC which provides a base IRQ domain and platform MSI domain. This driver assumes that the IMSIC state is already initialized by the IMSIC early driver.

---
 drivers/irqchip/Makefile                   |   2 +-
 drivers/irqchip/irq-riscv-imsic-platform.c | 271 +++++++++++++++++++++
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/irq-riscv-imsic-platform.c
drivers/irqchip/Makefile +1 -1
index d714724387ce..abca445a3229 100644
@@ -95,7 +95,7 @@ obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
-obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o
+obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o irq-riscv-imsic-platform.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
drivers/irqchip/irq-riscv-imsic-platform.c new file +271 -0

271 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#13 lore raw

PATCH v10 10/15 irqchip/riscv-imsic: Add support for PCI MSI irqdomain

In reply to Anup Patel

The Linux PCI framework requires it's own dedicated MSI irqdomain so let us create PCI MSI irqdomain as child of the IMSIC base irqdomain.

---
 drivers/irqchip/Kconfig                    |  7 ++++
 drivers/irqchip/irq-riscv-imsic-platform.c | 48 ++++++++++++++++++++++
 drivers/irqchip/irq-riscv-imsic-state.h    |  1 +
 3 files changed, 56 insertions(+)
drivers/irqchip/Kconfig +7 -0
index bdd80716114d..c1d69b418dfb 100644
@@ -552,6 +552,13 @@ config RISCV_IMSIC
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_MSI_IRQ
 
+config RISCV_IMSIC_PCI
+	bool
+	depends on RISCV_IMSIC
+	depends on PCI
+	depends on PCI_MSI
+	default RISCV_IMSIC
+
 config EXYNOS_IRQ_COMBINER
 	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
 	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
drivers/irqchip/irq-riscv-imsic-platform.c +48 -0

72 lines not on this page · raw message

drivers/irqchip/irq-riscv-imsic-state.h +1 -0
index 3170018949a8..ff3c377b9b33 100644
@@ -31,6 +31,7 @@ struct imsic_priv {
 
 	/* IRQ domains (created by platform driver) */
 	struct irq_domain *base_domain;
+	struct irq_domain *pci_domain;
 	struct irq_domain *plat_domain;
 };
 
Signature · 1 line
2.34.1
author
#14 lore raw

PATCH v10 11/15 dt-bindings: interrupt-controller: Add RISC-V advanced PLIC

In reply to Anup Patel

We add DT bindings document for RISC-V advanced platform level interrupt controller (APLIC) defined by the RISC-V advanced interrupt architecture (AIA) specification.

---
 .../interrupt-controller/riscv,aplic.yaml     | 172 ++++++++++++++++++
 1 file changed, 172 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml new file +172 -0
new file mode 100644
index 000000000000..190a6499c932
@@ -0,0 +1,172 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Advanced Platform Level Interrupt Controller (APLIC)
+
+maintainers:
+  - Anup Patel <[email protected]>
+
+description:
+  The RISC-V advanced interrupt architecture (AIA) defines an advanced
+  platform level interrupt controller (APLIC) for handling wired interrupts
+  in a RISC-V platform. The RISC-V AIA specification can be found at
+  https://github.com/riscv/riscv-aia.
+
+  The RISC-V APLIC is implemented as hierarchical APLIC domains where all
+  interrupt sources connect to the root APLIC domain and a parent APLIC
+  domain can delegate interrupt sources to it's child APLIC domains. There
+  is one device tree node for each APLIC domain.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qemu,aplic
+      - const: riscv,aplic
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 16384
+    description:
+      Given APLIC domain directly injects external interrupts to a set of
+      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv,cpu-intc
+      node, which has a CPU node (i.e. RISC-V HART) as parent.
+
+  msi-parent:
+    description:
+      Given APLIC domain forwards wired interrupts as MSIs to a AIA incoming
+      message signaled interrupt controller (IMSIC). If both "msi-parent" and
+      "interrupts-extended" properties are present then it means the APLIC
+      domain supports both MSI mode and Direct mode in HW. In this case, the
+      APLIC driver has to choose between MSI mode or Direct mode.
+
+  riscv,num-sources:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 1023
+    description:
+      Specifies the number of wired interrupt sources supported by this
+      APLIC domain.
+
+  riscv,children:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 1024
+    items:
+      maxItems: 1
+    description:
+      A list of child APLIC domains for the given APLIC domain. Each child
+      APLIC domain is assigned a child index in increasing order, with the
+      first child APLIC domain assigned child index 0. The APLIC domain child
+      index is used by firmware to delegate interrupts from the given APLIC
+      domain to a particular child APLIC domain.
+
+  riscv,delegation:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 1024
+    items:
+      items:
+        - description: child APLIC domain phandle
+        - description: first interrupt number of the parent APLIC domain (inclusive)
+        - description: last interrupt number of the parent APLIC domain (inclusive)
+    description:
+      A interrupt delegation list where each entry is a triple consisting
+      of child APLIC domain phandle, first interrupt number of the parent
+      APLIC domain, and last interrupt number of the parent APLIC domain.
+      Firmware must configure interrupt delegation registers based on
+      interrupt delegation list.
+
+dependencies:
+  riscv,delegation: [ "riscv,children" ]
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - "#interrupt-cells"
+  - riscv,num-sources
+
+anyOf:
+  - required:
+      - interrupts-extended
+  - required:
+      - msi-parent
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example 1 (APLIC domains directly injecting interrupt to HARTs):
+
+    interrupt-controller@c000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu1_intc 11>,
+                            <&cpu2_intc 11>,
+                            <&cpu3_intc 11>,
+                            <&cpu4_intc 11>;
+      reg = <0xc000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+      riscv,children = <&aplic1>, <&aplic2>;
+      riscv,delegation = <&aplic1 1 63>;
+    };
+
+    aplic1: interrupt-controller@d000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu1_intc 9>,
+                            <&cpu2_intc 9>;
+      reg = <0xd000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+
+    aplic2: interrupt-controller@e000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu3_intc 9>,
+                            <&cpu4_intc 9>;
+      reg = <0xe000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+
+  - |
+    // Example 2 (APLIC domains forwarding interrupts as MSIs):
+
+    interrupt-controller@c000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      msi-parent = <&imsic_mlevel>;
+      reg = <0xc000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+      riscv,children = <&aplic3>;
+      riscv,delegation = <&aplic3 1 63>;
+    };
+
+    aplic3: interrupt-controller@d000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      msi-parent = <&imsic_slevel>;
+      reg = <0xd000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+...
Signature · 7 lines
2.34.1


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
author
#15 lore raw

PATCH v10 12/15 irqchip: Add RISC-V advanced PLIC driver for direct-mode

In reply to Anup Patel

The RISC-V advanced interrupt architecture (AIA) specification defines advanced platform-level interrupt controller (APLIC) which has two modes of operation: 1) Direct mode and 2) MSI mode.
(For more details, refer https://github.com/riscv/riscv-aia)

In APLIC direct-mode, wired interrupts are forwared to CPUs (or HARTs) as a local external interrupt.

We add a platform irqchip driver for the RISC-V APLIC direct-mode to support RISC-V platforms having only wired interrupts.

---
 drivers/irqchip/Kconfig                  |   5 +
 drivers/irqchip/Makefile                 |   1 +
 drivers/irqchip/irq-riscv-aplic-direct.c | 343 +++++++++++++++++++++++
 drivers/irqchip/irq-riscv-aplic-main.c   | 232 +++++++++++++++
 drivers/irqchip/irq-riscv-aplic-main.h   |  45 +++
 include/linux/irqchip/riscv-aplic.h      | 119 ++++++++
 6 files changed, 745 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-aplic-direct.c
 create mode 100644 drivers/irqchip/irq-riscv-aplic-main.c
 create mode 100644 drivers/irqchip/irq-riscv-aplic-main.h
 create mode 100644 include/linux/irqchip/riscv-aplic.h
drivers/irqchip/Kconfig +5 -0
index c1d69b418dfb..1996cc6f666a 100644
@@ -546,6 +546,11 @@ config SIFIVE_PLIC
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
+config RISCV_APLIC
+	bool
+	depends on RISCV
+	select IRQ_DOMAIN_HIERARCHY
+
 config RISCV_IMSIC
 	bool
 	depends on RISCV
drivers/irqchip/Makefile +1 -0
index abca445a3229..7f8289790ed8 100644
@@ -95,6 +95,7 @@ obj-$(CONFIG_QCOM_MPM)			+= irq-qcom-mpm.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
+obj-$(CONFIG_RISCV_APLIC)		+= irq-riscv-aplic-main.o irq-riscv-aplic-direct.o
 obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o irq-riscv-imsic-platform.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
drivers/irqchip/irq-riscv-aplic-direct.c new file +343 -0

343 lines not on this page · raw message

drivers/irqchip/irq-riscv-aplic-main.c new file +232 -0

232 lines not on this page · raw message

drivers/irqchip/irq-riscv-aplic-main.h new file +45 -0

45 lines not on this page · raw message

include/linux/irqchip/riscv-aplic.h new file +119 -0

119 lines not on this page · raw message

Signature · 7 lines
2.34.1


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
author
#16 lore raw

PATCH v10 13/15 irqchip/riscv-aplic: Add support for MSI-mode

In reply to Anup Patel

The RISC-V advanced platform-level interrupt controller (APLIC) has two modes of operation: 1) Direct mode and 2) MSI mode. (For more details, refer https://github.com/riscv/riscv-aia)

In APLIC MSI-mode, wired interrupts are forwared as message signaled interrupts (MSIs) to CPUs via IMSIC.

We extend the existing APLIC irqchip driver to support MSI-mode for RISC-V platforms having both wired interrupts and MSIs.

---
 drivers/irqchip/Kconfig                |   6 +
 drivers/irqchip/Makefile               |   1 +
 drivers/irqchip/irq-riscv-aplic-main.c |   2 +-
 drivers/irqchip/irq-riscv-aplic-main.h |   8 +
 drivers/irqchip/irq-riscv-aplic-msi.c  | 285 +++++++++++++++++++++++++
 5 files changed, 301 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/irq-riscv-aplic-msi.c
drivers/irqchip/Kconfig +6 -0
index 1996cc6f666a..7adc4dbe07ff 100644
@@ -551,6 +551,12 @@ config RISCV_APLIC
 	depends on RISCV
 	select IRQ_DOMAIN_HIERARCHY
 
+config RISCV_APLIC_MSI
+	bool
+	depends on RISCV_APLIC
+	select GENERIC_MSI_IRQ
+	default RISCV_APLIC
+
 config RISCV_IMSIC
 	bool
 	depends on RISCV
drivers/irqchip/Makefile +1 -0
index 7f8289790ed8..47995fdb2c60 100644
@@ -96,6 +96,7 @@ obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
 obj-$(CONFIG_RISCV_APLIC)		+= irq-riscv-aplic-main.o irq-riscv-aplic-direct.o
+obj-$(CONFIG_RISCV_APLIC_MSI)		+= irq-riscv-aplic-msi.o
 obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o irq-riscv-imsic-platform.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
drivers/irqchip/irq-riscv-aplic-main.c +1 -1
index 87450708a733..d1b342b66551 100644
@@ -205,7 +205,7 @@ static int aplic_probe(struct platform_device *pdev)
 		msi_mode = of_property_present(to_of_node(dev->fwnode),
 						"msi-parent");
 	if (msi_mode)
-		rc = -ENODEV;
+		rc = aplic_msi_setup(dev, regs);
 	else
 		rc = aplic_direct_setup(dev, regs);
 	if (rc) {
drivers/irqchip/irq-riscv-aplic-main.h +8 -0
index 474a04229334..78267ec58098 100644
@@ -41,5 +41,13 @@ void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode);
 int aplic_setup_priv(struct aplic_priv *priv, struct device *dev,
 		     void __iomem *regs);
 int aplic_direct_setup(struct device *dev, void __iomem *regs);
+#ifdef CONFIG_RISCV_APLIC_MSI
+int aplic_msi_setup(struct device *dev, void __iomem *regs);
+#else
+static inline int aplic_msi_setup(struct device *dev, void __iomem *regs)
+{
+	return -ENODEV;
+}
+#endif
 
 #endif
drivers/irqchip/irq-riscv-aplic-msi.c new file +285 -0

285 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#17 lore raw

PATCH v10 14/15 RISC-V: Select APLIC and IMSIC drivers

In reply to Anup Patel

The QEMU virt machine supports AIA emulation and we also have quite a few RISC-V platforms with AIA support under development so let us select APLIC and IMSIC drivers for all RISC-V platforms.

---
 arch/riscv/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
arch/riscv/Kconfig +2 -0
index d607ab0f7c6d..45c660f1219d 100644
@@ -153,6 +153,8 @@ config RISCV
 	select PCI_DOMAINS_GENERIC if PCI
 	select PCI_MSI if PCI
 	select RISCV_ALTERNATIVE if !XIP_KERNEL
+	select RISCV_APLIC
+	select RISCV_IMSIC
 	select RISCV_INTC
 	select RISCV_TIMER if RISCV_SBI
 	select SIFIVE_PLIC
Signature · 1 line
2.34.1
author
#18 lore raw

PATCH v10 15/15 MAINTAINERS: Add entry for RISC-V AIA drivers

In reply to Anup Patel

Add myself as maintainer for RISC-V AIA drivers including the RISC-V INTC driver which supports both AIA and non-AIA platforms.

---
 MAINTAINERS | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
MAINTAINERS +14 -0
index c335ff64ecf1..89f0e7905b59 100644
@@ -18412,6 +18412,20 @@ S:	Maintained
 F:	drivers/mtd/nand/raw/r852.c
 F:	drivers/mtd/nand/raw/r852.h
 
+RISC-V AIA DRIVERS
+M:	Anup Patel <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
+F:	Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
+F:	drivers/irqchip/irq-riscv-aplic-*.c
+F:	drivers/irqchip/irq-riscv-aplic-*.h
+F:	drivers/irqchip/irq-riscv-imsic-*.c
+F:	drivers/irqchip/irq-riscv-imsic-*.h
+F:	drivers/irqchip/irq-riscv-intc.c
+F:	include/linux/irqchip/riscv-aplic.h
+F:	include/linux/irqchip/riscv-imsic.h
+
 RISC-V ARCHITECTURE
 M:	Paul Walmsley <[email protected]>
 M:	Palmer Dabbelt <[email protected]>
Signature · 1 line
2.34.1
#19 lore raw
In reply to Anup Patel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

I think this could simplify/remove(?) the locking as well.

I realize that the series in v10, and coming with a change like this now might be a bit of a pain...

Finally, another question related to APLIC/IMSIC. AFAIU the memory map of the IMSIC regions are constrained by the APLIC, which requires a certain layout for MSI forwarding (group/hart/guest bits). Say that a system doesn't have an APLIC, couldn't the layout requirement be simplified?

Again, thanks for the hard work!
Björn

author
#20 lore raw
Quoted 56 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Quoted 6 lines

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.
2) All wired IRQs for APLIC MSI-mode will also target a fixed HART hence irq_set_affinity() won't work for wired IRQs as well.
3) Contemporary storage/networking drivers which use per-core queues use irq_set_affinity() on queue IRQs to balance across cores but this will fail.
4) HART hotplug breaks because kernel irq-subsystem can't migrate the IRQs (both MSIs and Wired) targeting HART X to another HART Y when the HART X goes down.

The idea of treating per-HART MSIs as separate IRQs has been discussed in the past. The current approach works nicely with all kernel use-cases at the cost of additional work on the driver side.

Also, the current approach is very similar to the ARM GICv3 driver where ITS LPIs across CPUs are treated as single IRQ.

Quoted 9 lines

I realize that the series in v10, and coming with a change like this now might be a bit of a pain...

Finally, another question related to APLIC/IMSIC. AFAIU the memory map of the IMSIC regions are constrained by the APLIC, which requires a certain layout for MSI forwarding (group/hart/guest bits). Say that a system doesn't have an APLIC, couldn't the layout requirement be simplified?

Yes, this is already taken care of in the current IMSIC driver based on feedback from Atish. We can certainly improve flexibility on the IMSIC driver side if some case is missed-out.

The APLIC driver is certainly very strict about the arrangement of IMSIC files so we do additional sanity checks on the APLIC driver side at the time of probing.

Quoted 4 lines

Again, thanks for the hard work!
Björn

Regards,
Anup

#21 lore raw

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Quoted 3 lines

2) All wired IRQs for APLIC MSI-mode will also target a fixed HART hence irq_set_affinity() won't work for wired IRQs as well.

I'm not following here. Why would APLIC put a constraint here? I had a look at the specs, and I didn't see anything supporting the current scheme explicitly.

Quoted 3 lines

3) Contemporary storage/networking drivers which use per-core queues use irq_set_affinity() on queue IRQs to balance across cores but this will fail.

Or via the the managed interrupts. But this is a non-issue, as pointed out in my reply to 1.

Quoted 3 lines

4) HART hotplug breaks because kernel irq-subsystem can't migrate the IRQs (both MSIs and Wired) targeting HART X to another HART Y when the HART X goes down.

Yes, we might end up in scenarios where we can't move to a certain target cpu, but I wouldn't expect that to be a common scenario.

Quoted 2 lines

The idea of treating per-HART MSIs as separate IRQs has been discussed in the past.

Aha! I tried to look for it in lore, but didn't find any. Could you point me to those discussions?

Quoted 2 lines

Also, the current approach is very similar to the ARM GICv3 driver where ITS LPIs across CPUs are treated as single IRQ.

I'm not familiar with the GIC. Is the GICv3 design similar to IMSIC? I had the impression that the GIC had a more advanced interrupt routing mechanism, than what IMSIC exposes. I think x86 APIC takes the 1-1 approach (the folks on the To: list definitely knows! ;-)).

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Björn

author
#22 lore raw
Quoted 80 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

Quoted 5 lines

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Quoted 8 lines
Quoted 4 lines

2) All wired IRQs for APLIC MSI-mode will also target a fixed HART hence irq_set_affinity() won't work for wired IRQs as well.

I'm not following here. Why would APLIC put a constraint here? I had a look at the specs, and I didn't see anything supporting the current scheme explicitly.

Lets say the number of APLIC wired interrupts are greater than the number of per-CPU IMSIC IDs. In this case, if all wired interrupts are moved to a particular CPU then irq_set_affinity() will fail for some of the wired interrupts.

Quoted 12 lines
Quoted 4 lines

3) Contemporary storage/networking drivers which use per-core queues use irq_set_affinity() on queue IRQs to balance across cores but this will fail.

Or via the the managed interrupts. But this is a non-issue, as pointed out in my reply to 1.

Quoted 3 lines

4) HART hotplug breaks because kernel irq-subsystem can't migrate the IRQs (both MSIs and Wired) targeting HART X to another HART Y when the HART X goes down.

Quoted 8 lines

Yes, we might end up in scenarios where we can't move to a certain target cpu, but I wouldn't expect that to be a common scenario.

Quoted 2 lines

The idea of treating per-HART MSIs as separate IRQs has been discussed in the past.

Aha! I tried to look for it in lore, but didn't find any. Could you point me to those discussions?

This was done 2 years back in the AIA TG meeting when we were doing the PoC for AIA spec.

Quoted 8 lines
Quoted 3 lines

Also, the current approach is very similar to the ARM GICv3 driver where ITS LPIs across CPUs are treated as single IRQ.

I'm not familiar with the GIC. Is the GICv3 design similar to IMSIC? I had the impression that the GIC had a more advanced interrupt routing mechanism, than what IMSIC exposes. I think x86 APIC takes the 1-1 approach (the folks on the To: list definitely knows! ;-)).

GIC has a per-CPU redistributor which handles LPIs. The MSIs are taken by GIC ITS and forwarded as LPI to the redistributor of a CPU.

The GIC driver treats LPI numbering space as global and not per-CPU. Also, the limit on maximum number of LPIs is quite high because LPI INTID can be 32-bit wide.

Quoted 9 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

Regards,
Anup

#23 lore raw
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

2) All wired IRQs for APLIC MSI-mode will also target a fixed HART hence irq_set_affinity() won't work for wired IRQs as well.

Quoted 8 lines
Quoted 3 lines

I'm not following here. Why would APLIC put a constraint here? I had a look at the specs, and I didn't see anything supporting the current scheme explicitly.

Lets say the number of APLIC wired interrupts are greater than the number of per-CPU IMSIC IDs. In this case, if all wired interrupts are moved to a particular CPU then irq_set_affinity() will fail for some of the wired interrupts.

Right, it's the case of "full remote CPU" again. Thanks for clearing that up.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The idea of treating per-HART MSIs as separate IRQs has been discussed in the past.

Aha! I tried to look for it in lore, but didn't find any. Could you point me to those discussions?

This was done 2 years back in the AIA TG meeting when we were doing the PoC for AIA spec.

Ah, too bad. Thanks regardless.

Quoted 5 lines
Quoted 5 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

Quoted 7 lines
Quoted 3 lines

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Björn

author
#24 lore raw
Quoted 94 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Quoted 6 lines

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Quoted 37 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

2) All wired IRQs for APLIC MSI-mode will also target a fixed HART hence irq_set_affinity() won't work for wired IRQs as well.

Quoted 8 lines
Quoted 3 lines

I'm not following here. Why would APLIC put a constraint here? I had a look at the specs, and I didn't see anything supporting the current scheme explicitly.

Lets say the number of APLIC wired interrupts are greater than the number of per-CPU IMSIC IDs. In this case, if all wired interrupts are moved to a particular CPU then irq_set_affinity() will fail for some of the wired interrupts.

Right, it's the case of "full remote CPU" again. Thanks for clearing that up.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The idea of treating per-HART MSIs as separate IRQs has been discussed in the past.

Aha! I tried to look for it in lore, but didn't find any. Could you point me to those discussions?

This was done 2 years back in the AIA TG meeting when we were doing the PoC for AIA spec.

Ah, too bad. Thanks regardless.

Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

Quoted 6 lines
Quoted 6 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

Quoted 6 lines
Quoted 2 lines

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

In the RISC-V world, we can easily define a small fast track extension based on S*csrind extension which can allow a large number of IMSIC IDs per-core.

Instead of addressing problems on a hypothetical system, I suggest we go ahead with the current approach and deal with a system having MSI over-subscription when such a system shows up.

Regards,
Anup

#25 lore raw
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Some workloads never change the irq affinity.

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Quoted 8 lines

In the RISC-V world, we can easily define a small fast track extension based on S*csrind extension which can allow a large number of IMSIC IDs per-core.

Instead of addressing problems on a hypothetical system, I suggest we go ahead with the current approach and deal with a system having MSI over-subscription when such a system shows up.

I've pointed out my concerns. We're not agreeing, but hey, I'm just one sample point here! I'll leave it here for others to chime in!

Still much appreciate all the hard work on the series!

Have a nice weekend,
Björn

author
#26 lore raw
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Quoted 33 lines
Quoted 6 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Quoted 7 lines

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

Quoted 8 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 7 lines
Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Quoted 10 lines
Quoted 9 lines

In the RISC-V world, we can easily define a small fast track extension based on S*csrind extension which can allow a large number of IMSIC IDs per-core.

Instead of addressing problems on a hypothetical system, I suggest we go ahead with the current approach and deal with a system having MSI over-subscription when such a system shows up.

Quoted 4 lines

I've pointed out my concerns. We're not agreeing, but hey, I'm just one sample point here! I'll leave it here for others to chime in!

Still much appreciate all the hard work on the series!

Thanks, we have disagreements on this topic but this is certainly a good discussion.

Quoted 3 lines

Have a nice weekend,

Regards,
Anup

#27 lore raw
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 11 lines
Quoted 11 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Quoted 7 lines
Quoted 3 lines

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Yes, and while that is nice, it's not what IMSIC is.

Now, back to the weekend for real! ;-) (https://xkcd.com/386/)
Björn

#28 lore raw
Quoted 255 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 15 lines
Quoted 14 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Quoted 5 lines
Quoted 3 lines

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Björn

author
#29 lore raw
Quoted 262 lines · Björn Töpel
Quoted 259 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 18 lines
Quoted 7 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 7 lines
Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Quoted 6 lines
Quoted 1 line

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

In the 1-1 model, the ID on the current HART and target HART upon irq_set_affinity will be different so we can't leave the unused ID on current HART enabled because it can lead to spurious interrupts when the ID on current HART is re-used for some other device.

There is also a possibility of receiving an interrupt while the ID was moved to a new target HART in-which case we have to detect and re-trigger interrupt on the new target HART. In fact, x86 APLIC does an IPI to take care of this case.

Quoted 8 lines

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Yes, I have been evaluating the 1-1 model for the past few days. I also have a working implementation with a simple per-CPU bitmap based allocator which handles both legacy MSI (block of 1,2,4,8,16, or 32 IDs) and MSI-X.

The irq matrix allocator needs to be improved for handling legacy MSI so initially I will post a v11 series which works for me and converging with irq matrix allocator can be future work.

Regards,
Anup

#30 lore raw
Quoted 267 lines · Anup Patel
Quoted 266 lines · Björn Töpel
Quoted 255 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 4 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

Quoted 11 lines

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 15 lines
Quoted 14 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Quoted 5 lines
Quoted 3 lines

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

Quoted 7 lines
Quoted 2 lines

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

In the 1-1 model, the ID on the current HART and target HART upon irq_set_affinity will be different so we can't leave the unused ID on current HART enabled because it can lead to spurious interrupts when the ID on current HART is re-used for some other device.

Hmm, is this really an actual problem, or a theoretical one? The implementation need to track what's in-use, so can we ever get into this situation?

Somewhat related; I had a similar question for imsic_pci_{un,}mask_irq() -- why not only do the the default mask operation (only pci_msi_{un,}mask_irq()), but instead propagate to the IMSIC mask/unmask?

Quoted 4 lines

There is also a possibility of receiving an interrupt while the ID was moved to a new target HART in-which case we have to detect and re-trigger interrupt on the new target HART. In fact, x86 APLIC does an IPI to take care of this case.

This case I get, and the implementation can track that both are in use. It's the spurious one that I'm dubious of (don't get).

Quoted 9 lines
Quoted 8 lines

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Quoted 8 lines

Yes, I have been evaluating the 1-1 model for the past few days. I also have a working implementation with a simple per-CPU bitmap based allocator which handles both legacy MSI (block of 1,2,4,8,16, or 32 IDs) and MSI-X.

The irq matrix allocator needs to be improved for handling legacy MSI so initially I will post a v11 series which works for me and converging with irq matrix allocator can be future work.

What's missing/needs to be improved for legacy MSI (legacy MSI == !MSI-X, right?) in the matrix allocator?

Björn

author
#31 lore raw
Quoted 273 lines · Björn Töpel
Quoted 270 lines · Anup Patel
Quoted 268 lines · Björn Töpel
Quoted 255 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Quoted 15 lines · Anup Patel

The RISC-V AIA specification is ratified as-per the RISC-V international
process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and each MSI is dedicated to a certain hart. The series takes the approach to say that there are, e.g., 2047 interrupts ("lnx-irq") globally. Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt sources on "other" harts are pre-allocated. On the other hand it requires to propagate irq masking to other harts via IPIs (this is mostly done up setup/tear down). It's also wasteful, because msi-irqs are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core (or a sub-set of cores). The current scheme wastes a lot of msi-irqs. If we instead used a scheme where "msi-irq == lnx-irq", instead of "lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device would like to use 5 queues (5 cores) on a 128 core system, the current scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum 2047 MSIs for all cores (whole system). A better scheme, IMO, would be to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 15 lines
Quoted 14 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Quoted 5 lines
Quoted 3 lines

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

Quoted 8 lines
Quoted 4 lines

In the 1-1 model, the ID on the current HART and target HART upon irq_set_affinity will be different so we can't leave the unused ID on current HART enabled because it can lead to spurious interrupts when the ID on current HART is re-used for some other device.

Hmm, is this really an actual problem, or a theoretical one? The implementation need to track what's in-use, so can we ever get into this situation?

As of now, it is theoretical but it is certainly possible to hit this issue.

Quoted 5 lines

Somewhat related; I had a similar question for imsic_pci_{un,}mask_irq() -- why not only do the the default mask operation (only pci_msi_{un,}mask_irq()), but instead propagate to the IMSIC mask/unmask?

We have hierarchical IMSIC PCI irq domain whoes parent irq domain is IMSIC base domain. Unfortunately pci_msi_[un]mask_irq() don't work for hierarchical irq domain.

Quoted 23 lines
Quoted 5 lines

There is also a possibility of receiving an interrupt while the ID was moved to a new target HART in-which case we have to detect and re-trigger interrupt on the new target HART. In fact, x86 APLIC does an IPI to take care of this case.

This case I get, and the implementation can track that both are in use. It's the spurious one that I'm dubious of (don't get).

Quoted 14 lines
Quoted 8 lines

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Yes, I have been evaluating the 1-1 model for the past few days. I also have a working implementation with a simple per-CPU bitmap based allocator which handles both legacy MSI (block of 1,2,4,8,16, or 32 IDs) and MSI-X.

Quoted 6 lines
Quoted 3 lines

The irq matrix allocator needs to be improved for handling legacy MSI so initially I will post a v11 series which works for me and converging with irq matrix allocator can be future work.

What's missing/needs to be improved for legacy MSI (legacy MSI == !MSI-X, right?) in the matrix allocator?

For legacy MSI, a block IDs needs to be contiguous and the number of IDs can be a power of 2.

For a short difference between MSI vs MSI-X, refer:
https://en.wikipedia.org/wiki/Message_Signaled_Interrupts

Regards,
Anup

#32 lore raw
Quoted 279 lines · Anup Patel
Quoted 278 lines · Björn Töpel
Quoted 274 lines · Anup Patel
Quoted 262 lines · Björn Töpel
Quoted 259 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 62 lines · Anup Patel
Quoted 61 lines · Björn Töpel

Hi Anup,

Anup Patel <[email protected]> writes:

The RISC-V AIA specification is ratified as-per the RISC-V international
process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and
have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and
each MSI is dedicated to a certain hart. The series takes the approach
to say that there are, e.g., 2047 interrupts ("lnx-irq") globally.
Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the
slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt
sources on "other" harts are pre-allocated. On the other hand it
requires to propagate irq masking to other harts via IPIs (this is
mostly done up setup/tear down). It's also wasteful, because msi-irqs
are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core
(or a sub-set of cores). The current scheme wastes a lot of msi-irqs.
If we instead used a scheme where "msi-irq == lnx-irq", instead of
"lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be
a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device
would like to use 5 queues (5 cores) on a 128 core system, the current
scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts
on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of
interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no
broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum
2047 MSIs for all cores (whole system). A better scheme, IMO, would be
to expose 2047 * #harts unique MSIs.

Quoted 6 lines
Quoted 1 line

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each IRQ is not tied to a particular HART. This means we can't balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 18 lines
Quoted 7 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 7 lines
Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Quoted 6 lines
Quoted 1 line

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

In the 1-1 model, the ID on the current HART and target HART upon irq_set_affinity will be different so we can't leave the unused ID on current HART enabled because it can lead to spurious interrupts when the ID on current HART is re-used for some other device.

Quoted 5 lines
Quoted 3 lines

Hmm, is this really an actual problem, or a theoretical one? The implementation need to track what's in-use, so can we ever get into this situation?

As of now, it is theoretical but it is certainly possible to hit this issue.

Sorry for being slow here, Anup, but could you give an example how this could happen? For me it sounds like this could only be caused by a broken (buggy) implementation?

Quoted 8 lines
Quoted 4 lines

Somewhat related; I had a similar question for imsic_pci_{un,}mask_irq() -- why not only do the the default mask operation (only pci_msi_{un,}mask_irq()), but instead propagate to the IMSIC mask/unmask?

We have hierarchical IMSIC PCI irq domain whoes parent irq domain is IMSIC base domain. Unfortunately pci_msi_[un]mask_irq() don't work for hierarchical irq domain.

Ok! Thanks for the explanation!

Quoted 26 lines
Quoted 26 lines
Quoted 4 lines

There is also a possibility of receiving an interrupt while the ID was moved to a new target HART in-which case we have to detect and re-trigger interrupt on the new target HART. In fact, x86 APLIC does an IPI to take care of this case.

This case I get, and the implementation can track that both are in use. It's the spurious one that I'm dubious of (don't get).

Quoted 17 lines
Quoted 8 lines

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Yes, I have been evaluating the 1-1 model for the past few days. I also have a working implementation with a simple per-CPU bitmap based allocator which handles both legacy MSI (block of 1,2,4,8,16, or 32 IDs) and MSI-X.

The irq matrix allocator needs to be improved for handling legacy MSI so initially I will post a v11 series which works for me and converging with irq matrix allocator can be future work.

Quoted 5 lines
Quoted 2 lines

What's missing/needs to be improved for legacy MSI (legacy MSI == !MSI-X, right?) in the matrix allocator?

For legacy MSI, a block IDs needs to be contiguous and the number of IDs can be a power of 2.

Oh, so this is not supported by the matrix allocator?

Björn

author
#33 lore raw
Quoted 286 lines · Björn Töpel
Quoted 283 lines · Anup Patel
Quoted 281 lines · Björn Töpel
Quoted 267 lines · Anup Patel
Quoted 266 lines · Björn Töpel
Quoted 255 lines · Björn Töpel
Quoted 157 lines · Anup Patel
Quoted 149 lines · Björn Töpel
Quoted 98 lines · Anup Patel
Quoted 97 lines · Björn Töpel
Quoted 84 lines · Anup Patel
Quoted 83 lines · Björn Töpel

Thanks for the quick reply!

Quoted 68 lines · Anup Patel

On Thu, Oct 19, 2023 at 7:13 PM Björn Töpel <[email protected]> wrote:

Hi Anup,

Anup Patel <[email protected]> writes:

The RISC-V AIA specification is ratified as-per the RISC-V international
process. The latest ratified AIA specifcation can be found at:
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf

At a high-level, the AIA specification adds three things:
1) AIA CSRs
- Improved local interrupt support
2) Incoming Message Signaled Interrupt Controller (IMSIC)
- Per-HART MSI controller
- Support MSI virtualization
- Support IPI along with virtualization
3) Advanced Platform-Level Interrupt Controller (APLIC)
- Wired interrupt controller
- In MSI-mode, converts wired interrupt into MSIs (i.e. MSI generator)
- In Direct-mode, injects external interrupts directly into HARTs

Thanks for working on the AIA support! I had a look at the series, and
have some concerns about interrupt ID abstraction.

A bit of background, for readers not familiar with the AIA details.

IMSIC allows for 2047 unique MSI ("msi-irq") sources per hart, and
each MSI is dedicated to a certain hart. The series takes the approach
to say that there are, e.g., 2047 interrupts ("lnx-irq") globally.
Each lnx-irq consists of #harts * msi-irq -- a slice -- and in the
slice only *one* msi-irq is acutally used.

This scheme makes affinity changes more robust, because the interrupt
sources on "other" harts are pre-allocated. On the other hand it
requires to propagate irq masking to other harts via IPIs (this is
mostly done up setup/tear down). It's also wasteful, because msi-irqs
are hogged, and cannot be used.

Contemporary storage/networking drivers usually uses queues per core
(or a sub-set of cores). The current scheme wastes a lot of msi-irqs.
If we instead used a scheme where "msi-irq == lnx-irq", instead of
"lnq-irq = {hart 0;msi-irq x , ... hart N;msi-irq x}", there would be
a lot MSIs for other users. 1-1 vs 1-N. E.g., if a storage device
would like to use 5 queues (5 cores) on a 128 core system, the current
scheme would consume 5 * 128 MSIs, instead of just 5.

On the plus side:
* Changing interrupts affinity will never fail, because the interrupts
on each hart is pre-allocated.

On the negative side:
* Wasteful interrupt usage, and a system can potientially "run out" of
interrupts. Especially for many core systems.
* Interrupt masking need to proagate to harts via IPIs (there's no
broadcast csr in IMSIC), and a more complex locking scheme IMSIC

Summary:
The current series caps the number of global interrupts to maximum
2047 MSIs for all cores (whole system). A better scheme, IMO, would be
to expose 2047 * #harts unique MSIs.

I think this could simplify/remove(?) the locking as well.

Exposing 2047 * #harts unique MSIs has multiple issues:
1) The irq_set_affinity() does not work for MSIs because each
IRQ is not tied to a particular HART. This means we can't
balance the IRQ processing load among HARTs.

Yes, you can balance. In your code, each *active* MSI is still bound/active to a specific hard together with the affinity mask. In an 1-1 model you would still need to track the affinity mask, but the irq_set_affinity() would be different. It would try to allocate a new MSI from the target CPU, and then switch to having that MSI active.

That's what x86 does AFAIU, which is also constrained by the # of available MSIs.

Quoted 6 lines
Quoted 2 lines

The downside, as I pointed out, is that the set affinity action can fail for a certain target CPU.

Yes, irq_set_affinity() can fail for the suggested approach plus for RISC-V AIA, one HART does not have access to other HARTs MSI enable/disable bits so the approach will also involve IPI.

Correct, but the current series does a broadcast to all cores, where the 1-1 approach is at most an IPI to a single core.

Quoted 6 lines
Quoted 3 lines

128+c machines are getting more common, and you have devices that you bring up/down on a per-core basis. Broadcasting IPIs to all cores, when dealing with a per-core activity is a pretty noisy neighbor.

Broadcast IPI in the current approach is only done upon MSI mask/unmask operation. It is not done upon set_affinity() of interrupt handling.

I'm aware. We're on the same page here.

Quoted 9 lines
Quoted 3 lines

This could be fixed in the existing 1-n approach, by not require to sync the cores that are not handling the MSI in question. "Lazy disable"

Incorrect. The approach you are suggesting involves an IPI upon every irq_set_affinity(). This is because a HART can only enable it's own MSI ID so when an IRQ is moved to from HART A to HART B with a different ID X on HART B then we will need an IPI in irq_set_affinit() to enable ID X on HART B.

Yes, the 1-1 approach will require an IPI to one target cpu on affinity changes, and similar on mask/unmask.

The 1-n approach, require no-IPI on affinity changes (nice!), but IPI broadcast to all cores on mask/unmask (not so nice).

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

My concern is interrupts become a scarce resource with this implementation, but maybe my view is incorrect. I've seen bare-metal x86 systems (no VMs) with ~200 cores, and ~2000 interrupts, but maybe that is considered "a lot of interrupts".

As long as we don't get into scenarios where we're running out of interrupts, due to the software design.

Quoted 7 lines
Quoted 3 lines

The current approach is simpler and ensures irq_set_affinity always works. The limit of max 2047 IDs is sufficient for many systems (if not all).

Let me give you another view. On a 128c system each core has ~16 unique interrupts for disposal. E.g. the Intel E800 NIC has more than 2048 network queue pairs for each PF.

Quoted 5 lines

Clearly, this example is a hypothetical and represents a poorly designed platform.

Having just 16 IDs per-Core is a very poor design choice. In fact, the Server SoC spec mandates a minimum 255 IDs.

Quoted 6 lines

You are misreading. A 128c system with 2047 MSIs per-core, will only have 16 *per-core unique* (2047/128) interrupts with the current series.

I'm not saying that each IMSIC has 16 IDs, I'm saying that in a 128c system with the maximum amount of MSIs possible in the spec, you'll end up with 16 *unique* interrupts per core.

Quoted 5 lines

-ENOPARSE

I don't see how this applies to the current approach because we treat MSI ID space as global across cores so if a system has 2047 MSIs per-core then we have 2047 MSIs across all cores.

Ok, I'll try again! :-)

Let's assume that each core in the 128c system has some per-core resources, say a two NIC queue pairs, and a storage queue pair. This will consume, e.g., 2*2 + 2 (6) MSI sources from the global namespace.

If each core does this it'll be 6*128 MSI sources of the global namespace.

The maximum number of "privates" MSI sources a core can utilize is 16.

I'm trying (it's does seem to go that well ;-)) to point out that it's only 16 unique sources per core. For, say, a 256 core system it would be
8. 2047 MSI sources in a system is not much.

Say that I want to spin up 24 NIC queues with one MSI each on each core on my 128c system. That's not possible with this series, while with an 1-1 system it wouldn't be an issue.

Clearer, or still weird?

Quoted 40 lines
Quoted 40 lines
Quoted 7 lines

Regarding NICs which support a large number of queues, the driver will typically enable only one queue per-core and set the affinity to separate cores. We have user-space data plane applications based on DPDK which are capable of using a large number of NIC queues but these applications are polling based and don't use MSIs.

That's one sample point, and clearly not the only one. There are *many* different usage models. Just because you *assign* MSI, doesn't mean they are firing all the time.

I can show you a couple of networking setups where this is clearly not enough. Each core has a large number of QoS queues, and each queue would very much like to have a dedicated MSI.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

When we encounter a system requiring a large number of MSIs, we can either:
1) Extend the AIA spec to support greater than 2047 IDs
2) Re-think the approach in the IMSIC driver

The choice between #1 and #2 above depends on the
guarantees we want for irq_set_affinity().

The irq_set_affinity() behavior is better with this series, but I think the other downsides: number of available interrupt sources, and IPI broadcast are worse.

Quoted 5 lines

The IPI overhead in the approach you are suggesting will be even bad compared to the IPI overhead of the current approach because we will end-up doing IPI upon every irq_set_affinity() in the suggested approach compared to doing IPI upon every mask/unmask in the current approach.

Again, very workload dependent.

This series does IPI broadcast on masking/unmasking, which means that cores that don't care get interrupted because, say, a network queue-pair is setup on another core.

Quoted 8 lines
Quoted 1 line

Some workloads never change the irq affinity.

There are various events which irq affinity such as irq balance, CPU hotplug, system suspend, etc.

Also, the 1-1 approach does IPI upon set_affinity, mask and unmask whereas the 1-n approach does IPI only upon mask and unmask.

An important distinction; When you say IPI on mask/unmask it is a broadcast IPI to *all* cores, which is pretty instrusive.

The 1-1 variant does an IPI to a *one* target core.

Quoted 15 lines
Quoted 14 lines

I'm just pointing out that there are pro/cons with both variants.

Quoted 2 lines

The biggest advantage of the current approach is a reliable irq_set_affinity() which is a very valuable thing to have.

...and I'm arguing that we're paying a big price for that.

Quoted 3 lines

ARM systems easily support a large number of LPIs per-core.
For example, GIC-700 supports 56000 LPIs per-core.
(Refer, https://developer.arm.com/documentation/101516/0300/About-the-GIC-700/Features)

Yeah, but this is not the GIC. This is something that looks more like the x86 world. We'll be stuck with a lot of implementations with AIA 1.0 spec, and many cores.

Quoted 5 lines
Quoted 3 lines

Well, RISC-V AIA is neigher ARM GIG not x86 APIC. All I am saying is that there are systems with large number per-core interrupt IDs for handling MSIs.

Yes, and while that is nice, it's not what IMSIC is.

Some follow-ups, after thinking more about it more over the weekend.

Quoted 7 lines
Quoted 2 lines

* Do one really need an IPI for irq_set_affinity() for the 1-1 model? Why touch the enable/disable bits when moving interrupts?

In the 1-1 model, the ID on the current HART and target HART upon irq_set_affinity will be different so we can't leave the unused ID on current HART enabled because it can lead to spurious interrupts when the ID on current HART is re-used for some other device.

Hmm, is this really an actual problem, or a theoretical one? The implementation need to track what's in-use, so can we ever get into this situation?

Quoted 5 lines
Quoted 1 line

As of now, it is theoretical but it is certainly possible to hit this issue.

Sorry for being slow here, Anup, but could you give an example how this could happen? For me it sounds like this could only be caused by a broken (buggy) implementation?

Let me re-state the problem: When ID X on HART A is moved to ID Y on HART B. Now the movement might have be initiated by some some HART C doing irq_set_affinity(). Once the movement is done the ID X on HART A should be disabled because if not disabled then some device (possibly buggy) can have HART A to take MSI with ID X but this MSI won't map to any Linux IRQ since the it is already moved to ID Y on HART B.

Quoted 38 lines
Quoted 9 lines
Quoted 5 lines

Somewhat related; I had a similar question for imsic_pci_{un,}mask_irq() -- why not only do the the default mask operation (only pci_msi_{un,}mask_irq()), but instead propagate to the IMSIC mask/unmask?

We have hierarchical IMSIC PCI irq domain whoes parent irq domain is IMSIC base domain. Unfortunately pci_msi_[un]mask_irq() don't work for hierarchical irq domain.

Ok! Thanks for the explanation!

Quoted 26 lines
Quoted 26 lines
Quoted 4 lines

There is also a possibility of receiving an interrupt while the ID was moved to a new target HART in-which case we have to detect and re-trigger interrupt on the new target HART. In fact, x86 APLIC does an IPI to take care of this case.

This case I get, and the implementation can track that both are in use. It's the spurious one that I'm dubious of (don't get).

Quoted 17 lines
Quoted 8 lines

* In my book the IMSIC looks very much like the x86 LAPIC, which also has few interrupts (IMSIC <2048, LAPIC 256). The IRQ matrix allocator [1], and a scheme similar to LAPIC [2] would be a good fit. This is the 1-1 model, but more sophisticated than what I've been describing (e.g. properly handling mangaged/regular irqs). As a bonus we would get the IRQ matrix debugfs/tracepoint support.

Yes, I have been evaluating the 1-1 model for the past few days. I also have a working implementation with a simple per-CPU bitmap based allocator which handles both legacy MSI (block of 1,2,4,8,16, or 32 IDs) and MSI-X.

The irq matrix allocator needs to be improved for handling legacy MSI so initially I will post a v11 series which works for me and converging with irq matrix allocator can be future work.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

What's missing/needs to be improved for legacy MSI (legacy MSI == !MSI-X, right?) in the matrix allocator?

For legacy MSI, a block IDs needs to be contiguous and the number of IDs can be a power of 2.

Oh, so this is not supported by the matrix allocator?

Yes, that's correct. IRQ matrix allocator only supports allocating IDs suitable for MSI-X. Improving IRQ matrix allocator is a separate effort in my opinion.

The ARM GICv3/v4 driver supports both MSI and MSI-X.

Regards,
Anup