[PATCH v4 0/2] Loongarch irq-redirect supprot

[PATCH v4 0/2] Loongarch irq-redirect supprot

[Tianyang Zhang]

From: Super User <root@localhost.localdomain>

This series of patches introduces support for interrupt-redirect
controllers, and this hardware feature will be supported on 3C6000
for the first time

change log:
	v0->v1:
	1.Rename the model names in the document.
	2.Adjust the code format.
	3.Remove architecture - specific prefixes.
	4.Refactor the initialization logic, and IR driver no longer set AVEC_ENABLE.
	5.Enhance compatibility under certain configurations.

	v1->v2:
	1.Fixed an erroneous enabling issue.

	v2->v3
	1.Replace smp_call with address mapping to access registers
	2.Fix some code style issues

	v3->v4
	1.Provide reasonable comments on the modifications made to IRQ_SET_MASK_OK_DONE	
	2.Replace meaningless empty functions with parent_mask/unmask/ack
	3.Added and indeed released resources
	4.Added judgment for data structure initialization completion to avoid duplicate creation during cpuhotplug
	5.Fixed the code style and some unnecessary troubles

Tianyang Zhang (2):
  Docs/LoongArch: Add Advanced Extended-Redirect IRQ model description
  irq/irq-loongarch-ir:Add Redirect irqchip support

 .../arch/loongarch/irq-chip-model.rst         |  38 ++
 .../zh_CN/arch/loongarch/irq-chip-model.rst   |  37 ++
 arch/loongarch/include/asm/cpu-features.h     |   1 +
 arch/loongarch/include/asm/cpu.h              |   2 +
 arch/loongarch/include/asm/loongarch.h        |   6 +
 arch/loongarch/kernel/cpu-probe.c             |   2 +
 drivers/irqchip/Makefile                      |   2 +-
 drivers/irqchip/irq-loongarch-avec.c          |  25 +-
 drivers/irqchip/irq-loongarch-ir.c            | 562 ++++++++++++++++++
 drivers/irqchip/irq-loongson.h                |  12 +
 include/linux/cpuhotplug.h                    |   1 +
 11 files changed, 674 insertions(+), 14 deletions(-)
 create mode 100644 drivers/irqchip/irq-loongarch-ir.c

-- 
2.41.0

[PATCH v4 1/2] Docs/LoongArch: Add Advanced Extended-Redirect IRQ model description

[Tianyang Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 4520 bytes --]

Introduce the redirect interrupt controllers.When the redirect interrupt
controller is enabled, the routing target of MSI interrupts is no longer a
specific CPU and vector number, but a specific redirect entry. The actual
CPU and vector number used are described by the redirect entry.

Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
---
 .../arch/loongarch/irq-chip-model.rst         | 38 +++++++++++++++++++
 .../zh_CN/arch/loongarch/irq-chip-model.rst   | 37 ++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/Documentation/arch/loongarch/irq-chip-model.rst b/Documentation/arch/loongarch/irq-chip-model.rst
index a7ecce11e445..d9a2e8d7f70e 100644
--- a/Documentation/arch/loongarch/irq-chip-model.rst
+++ b/Documentation/arch/loongarch/irq-chip-model.rst
@@ -181,6 +181,44 @@ go to PCH-PIC/PCH-LPC and gathered by EIOINTC, and then go to CPUINTC directly::
              | Devices |
              +---------+
 
+Advanced Extended IRQ model (with redirection)
+==============================================
+
+In this model, IPI (Inter-Processor Interrupt) and CPU Local Timer interrupt go
+to CPUINTC directly, CPU UARTS interrupts go to LIOINTC, PCH-MSI interrupts go
+to REDIRECT for remapping it to AVEC, and then go to CPUINTC directly, while all
+other devices interrupts go to PCH-PIC/PCH-LPC and gathered by EIOINTC, and then
+go to CPUINTC directly::
+
+ +-----+     +-----------------------+     +-------+
+ | IPI | --> |        CPUINTC        | <-- | Timer |
+ +-----+     +-----------------------+     +-------+
+              ^          ^          ^
+              |          |          |
+       +---------+ +----------+ +---------+     +-------+
+       | EIOINTC | | AVECINTC | | LIOINTC | <-- | UARTs |
+       +---------+ +----------+ +---------+     +-------+
+            ^            ^
+            |            |
+            |      +----------+
+            |      | REDIRECT |
+            |      +----------+
+            |            ^
+            |            |
+       +---------+  +---------+
+       | PCH-PIC |  | PCH-MSI |
+       +---------+  +---------+
+         ^     ^           ^
+         |     |           |
+ +---------+ +---------+ +---------+
+ | Devices | | PCH-LPC | | Devices |
+ +---------+ +---------+ +---------+
+                  ^
+                  |
+             +---------+
+             | Devices |
+             +---------+
+
 ACPI-related definitions
 ========================
 
diff --git a/Documentation/translations/zh_CN/arch/loongarch/irq-chip-model.rst b/Documentation/translations/zh_CN/arch/loongarch/irq-chip-model.rst
index d4ff80de47b6..7e4e3e55c7ad 100644
--- a/Documentation/translations/zh_CN/arch/loongarch/irq-chip-model.rst
+++ b/Documentation/translations/zh_CN/arch/loongarch/irq-chip-model.rst
@@ -174,6 +174,43 @@ CPU串口（UARTs）中断发送到LIOINTC，PCH-MSI中断发送到AVECINTC，
              | Devices |
              +---------+
 
+高级扩展IRQ模型 (带重定向)
+==========================
+
+在这种模型里面，IPI（Inter-Processor Interrupt）和CPU本地时钟中断直接发送到CPUINTC，
+CPU串口（UARTs）中断发送到LIOINTC，PCH-MSI中断首先发送到REDIRECT模块,完成重定向后发
+送到AVECINTC，而后通过AVECINTC直接送达CPUINTC，而其他所有设备的中断则分别发送到所连
+接的PCH-PIC/PCH-LPC，然后由EIOINTC统一收集，再直接到达CPUINTC::
+
+ +-----+     +-----------------------+     +-------+
+ | IPI | --> |        CPUINTC        | <-- | Timer |
+ +-----+     +-----------------------+     +-------+
+              ^          ^          ^
+              |          |          |
+       +---------+ +----------+ +---------+     +-------+
+       | EIOINTC | | AVECINTC | | LIOINTC | <-- | UARTs |
+       +---------+ +----------+ +---------+     +-------+
+            ^            ^
+            |            |
+            |      +----------+
+            |      | REDIRECT |
+            |      +----------+
+            |            ^
+            |            |
+       +---------+  +---------+
+       | PCH-PIC |  | PCH-MSI |
+       +---------+  +---------+
+         ^     ^           ^
+         |     |           |
+ +---------+ +---------+ +---------+
+ | Devices | | PCH-LPC | | Devices |
+ +---------+ +---------+ +---------+
+                  ^
+                  |
+             +---------+
+             | Devices |
+             +---------+
+
 ACPI相关的定义
 ==============
 
-- 
2.41.0

[PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Tianyang Zhang]

The main function of the Redirected interrupt controller is to manage the
redirected-interrupt table, which consists of many redirected entries.
When MSI interrupts are requested, the driver creates a corresponding
redirected entry that describes the target CPU/vector number and the
operating mode of the interrupt. The redirected interrupt module has an
independent cache, and during the interrupt routing process, it will
prioritize the redirected entries that hit the cache. The driver
invalidates certain entry caches via a command queue.

Co-developed-by: Liupu Wang <wangliupu@loongson.cn>
Signed-off-by: Liupu Wang <wangliupu@loongson.cn>
Signed-off-by: Tianyang Zhang <zhangtianyang@loongson.cn>
---
 arch/loongarch/include/asm/cpu-features.h |   1 +
 arch/loongarch/include/asm/cpu.h          |   2 +
 arch/loongarch/include/asm/loongarch.h    |   6 +
 arch/loongarch/kernel/cpu-probe.c         |   2 +
 drivers/irqchip/Makefile                  |   2 +-
 drivers/irqchip/irq-loongarch-avec.c      |  25 +-
 drivers/irqchip/irq-loongarch-ir.c        | 562 ++++++++++++++++++++++
 drivers/irqchip/irq-loongson.h            |  12 +
 include/linux/cpuhotplug.h                |   1 +
 9 files changed, 599 insertions(+), 14 deletions(-)
 create mode 100644 drivers/irqchip/irq-loongarch-ir.c

diff --git a/arch/loongarch/include/asm/cpu-features.h b/arch/loongarch/include/asm/cpu-features.h
index fc83bb32f9f0..03f7e93e81e0 100644
--- a/arch/loongarch/include/asm/cpu-features.h
+++ b/arch/loongarch/include/asm/cpu-features.h
@@ -68,5 +68,6 @@
 #define cpu_has_ptw		cpu_opt(LOONGARCH_CPU_PTW)
 #define cpu_has_lspw		cpu_opt(LOONGARCH_CPU_LSPW)
 #define cpu_has_avecint		cpu_opt(LOONGARCH_CPU_AVECINT)
+#define cpu_has_redirectint	cpu_opt(LOONGARCH_CPU_REDIRECTINT)
 
 #endif /* __ASM_CPU_FEATURES_H */
diff --git a/arch/loongarch/include/asm/cpu.h b/arch/loongarch/include/asm/cpu.h
index 98cf4d7b4b0a..33cd96e569d8 100644
--- a/arch/loongarch/include/asm/cpu.h
+++ b/arch/loongarch/include/asm/cpu.h
@@ -102,6 +102,7 @@ enum cpu_type_enum {
 #define CPU_FEATURE_PTW			27	/* CPU has hardware page table walker */
 #define CPU_FEATURE_LSPW		28	/* CPU has LSPW (lddir/ldpte instructions) */
 #define CPU_FEATURE_AVECINT		29	/* CPU has AVEC interrupt */
+#define CPU_FEATURE_REDIRECTINT		30      /* CPU has interrupt remmap */
 
 #define LOONGARCH_CPU_CPUCFG		BIT_ULL(CPU_FEATURE_CPUCFG)
 #define LOONGARCH_CPU_LAM		BIT_ULL(CPU_FEATURE_LAM)
@@ -133,5 +134,6 @@ enum cpu_type_enum {
 #define LOONGARCH_CPU_PTW		BIT_ULL(CPU_FEATURE_PTW)
 #define LOONGARCH_CPU_LSPW		BIT_ULL(CPU_FEATURE_LSPW)
 #define LOONGARCH_CPU_AVECINT		BIT_ULL(CPU_FEATURE_AVECINT)
+#define LOONGARCH_CPU_REDIRECTINT	BIT_ULL(CPU_FEATURE_REDIRECTINT)
 
 #endif /* _ASM_CPU_H */
diff --git a/arch/loongarch/include/asm/loongarch.h b/arch/loongarch/include/asm/loongarch.h
index 52651aa0e583..95e06cb6831e 100644
--- a/arch/loongarch/include/asm/loongarch.h
+++ b/arch/loongarch/include/asm/loongarch.h
@@ -1130,6 +1130,7 @@
 #define  IOCSRF_FLATMODE		BIT_ULL(10)
 #define  IOCSRF_VM			BIT_ULL(11)
 #define  IOCSRF_AVEC			BIT_ULL(15)
+#define  IOCSRF_REDIRECTINT		BIT_ULL(16)
 
 #define LOONGARCH_IOCSR_VENDOR		0x10
 
@@ -1189,6 +1190,11 @@
 
 #define LOONGARCH_IOCSR_EXTIOI_NODEMAP_BASE	0x14a0
 #define LOONGARCH_IOCSR_EXTIOI_IPMAP_BASE	0x14c0
+#define LOONGARCH_IOCSR_REDIRECT_CFG		0x15e0
+#define LOONGARCH_IOCSR_REDIRECT_TBR		0x15e8  /* IRT BASE REG*/
+#define LOONGARCH_IOCSR_REDIRECT_CQB		0x15f0  /* IRT CACHE QUEUE BASE */
+#define LOONGARCH_IOCSR_REDIRECT_CQH		0x15f8  /* IRT CACHE QUEUE HEAD, 32bit */
+#define LOONGARCH_IOCSR_REDIRECT_CQT		0x15fc  /* IRT CACHE QUEUE TAIL, 32bit */
 #define LOONGARCH_IOCSR_EXTIOI_EN_BASE		0x1600
 #define LOONGARCH_IOCSR_EXTIOI_BOUNCE_BASE	0x1680
 #define LOONGARCH_IOCSR_EXTIOI_ISR_BASE		0x1800
diff --git a/arch/loongarch/kernel/cpu-probe.c b/arch/loongarch/kernel/cpu-probe.c
index fedaa67cde41..543474fd1399 100644
--- a/arch/loongarch/kernel/cpu-probe.c
+++ b/arch/loongarch/kernel/cpu-probe.c
@@ -289,6 +289,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_loongarch *c, unsigned int
 		c->options |= LOONGARCH_CPU_EIODECODE;
 	if (config & IOCSRF_AVEC)
 		c->options |= LOONGARCH_CPU_AVECINT;
+	if (config & IOCSRF_REDIRECTINT)
+		c->options |= LOONGARCH_CPU_REDIRECTINT;
 	if (config & IOCSRF_VM)
 		c->options |= LOONGARCH_CPU_HYPERVISOR;
 }
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 365bcea9a61f..2bb8618f96d1 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -114,7 +114,7 @@ obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
 obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
 obj-$(CONFIG_TI_PRUSS_INTC)		+= irq-pruss-intc.o
-obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o irq-loongarch-avec.o
+obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o irq-loongarch-avec.o irq-loongarch-ir.o
 obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o
 obj-$(CONFIG_LOONGSON_EIOINTC)		+= irq-loongson-eiointc.o
 obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
diff --git a/drivers/irqchip/irq-loongarch-avec.c b/drivers/irqchip/irq-loongarch-avec.c
index 80e55955a29f..4e114c8ca3d0 100644
--- a/drivers/irqchip/irq-loongarch-avec.c
+++ b/drivers/irqchip/irq-loongarch-avec.c
@@ -24,7 +24,6 @@
 #define VECTORS_PER_REG		64
 #define IRR_VECTOR_MASK		0xffUL
 #define IRR_INVALID_MASK	0x80000000UL
-#define AVEC_MSG_OFFSET		0x100000
 
 #ifdef CONFIG_SMP
 struct pending_list {
@@ -47,15 +46,6 @@ struct avecintc_chip {
 
 static struct avecintc_chip loongarch_avec;
 
-struct avecintc_data {
-	struct list_head	entry;
-	unsigned int		cpu;
-	unsigned int		vec;
-	unsigned int		prev_cpu;
-	unsigned int		prev_vec;
-	unsigned int		moving;
-};
-
 static inline void avecintc_enable(void)
 {
 	u64 value;
@@ -85,7 +75,7 @@ static inline void pending_list_init(int cpu)
 	INIT_LIST_HEAD(&plist->head);
 }
 
-static void avecintc_sync(struct avecintc_data *adata)
+void avecintc_sync(struct avecintc_data *adata)
 {
 	struct pending_list *plist;
 
@@ -109,7 +99,12 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
 			return -EBUSY;
 
 		if (cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
-			return 0;
+			/*
+			 * The new affinity configuration has taken effect,
+			 * and returning IRQ_SET_MASK_OK_DONE here indicates that no further
+			 * changes need to be made in the subsequent process
+			 */
+			return IRQ_SET_MASK_OK_DONE;
 
 		cpumask_and(&intersect_mask, dest, cpu_online_mask);
 
@@ -121,7 +116,8 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
 		adata->cpu = cpu;
 		adata->vec = vector;
 		per_cpu_ptr(irq_map, adata->cpu)[adata->vec] = irq_data_to_desc(data);
-		avecintc_sync(adata);
+		if (!cpu_has_redirectint)
+			avecintc_sync(adata);
 	}
 
 	irq_data_update_effective_affinity(data, cpumask_of(cpu));
@@ -412,6 +408,9 @@ static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
 
 static inline int __init acpi_cascade_irqdomain_init(void)
 {
+	if (cpu_has_redirectint)
+		return redirect_acpi_init(loongarch_avec.domain);
+
 	return acpi_table_parse_madt(ACPI_MADT_TYPE_MSI_PIC, pch_msi_parse_madt, 1);
 }
 
diff --git a/drivers/irqchip/irq-loongarch-ir.c b/drivers/irqchip/irq-loongarch-ir.c
new file mode 100644
index 000000000000..ae42ec5028d2
--- /dev/null
+++ b/drivers/irqchip/irq-loongarch-ir.c
@@ -0,0 +1,562 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Loongson Technologies, Inc.
+ */
+
+#include <linux/cpuhotplug.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/spinlock.h>
+#include <linux/msi.h>
+
+#include <asm/irq.h>
+#include <asm/loongarch.h>
+#include <asm/setup.h>
+#include <larchintrin.h>
+
+#include "irq-loongson.h"
+#include "irq-msi-lib.h"
+
+#define IRD_ENTRIES			65536
+
+/* redirect entry size 128bits */
+#define IRD_PAGE_ORDER			(20 - PAGE_SHIFT)
+
+/* irt cache invalid queue */
+#define	INVALID_QUEUE_SIZE		4096
+
+#define INVALID_QUEUE_PAGE_ORDER	(16 - PAGE_SHIFT)
+
+#define GPID_ADDR_MASK			0x3ffffffffffULL
+#define GPID_ADDR_SHIFT			6
+
+#define CQB_SIZE_SHIFT			0
+#define CQB_SIZE_MASK			0xf
+#define CQB_ADDR_SHIFT			12
+#define CQB_ADDR_MASK			(0xfffffffffULL)
+
+#define CFG_DISABLE_IDLE		2
+#define INVALID_INDEX			0
+
+#define MAX_IR_ENGINES			16
+
+struct irq_domain *redirect_domain;
+
+struct redirect_entry {
+	struct  {
+		__u64	valid	: 1,
+			res1	: 5,
+			gpid	: 42,
+			res2	: 8,
+			vector	: 8;
+	}	lo;
+	__u64	hi;
+};
+
+struct redirect_gpid {
+	u64	pir[4];      /* Pending interrupt requested */
+	u8	en	: 1, /* doorbell */
+		res0	: 7;
+	u8	irqnum;
+	u16	res1;
+	u32	dst;
+	u32	rsvd[6];
+} __aligned(64);
+
+struct redirect_table {
+	int			node;
+	struct redirect_entry	*table;
+	unsigned long		*bitmap;
+	unsigned int		nr_ird;
+	struct page		*page;
+	raw_spinlock_t		lock;
+};
+
+struct redirect_item {
+	int			index;
+	struct redirect_entry	*entry;
+	struct redirect_gpid	*gpid;
+	struct redirect_table	*table;
+};
+
+struct redirect_queue {
+	int		node;
+	u64		base;
+	u32		max_size;
+	int		head;
+	int		tail;
+	struct page	*page;
+	raw_spinlock_t	lock;
+};
+
+struct irde_desc {
+	struct	redirect_table	ird_table;
+	struct	redirect_queue	inv_queue;
+	int	finish;
+};
+
+struct irde_inv_cmd {
+	union {
+		__u64	cmd_info;
+		struct {
+			__u64	res1		: 4,
+				type		: 1,
+				need_notice	: 1,
+				pad		: 2,
+				index		: 16,
+				pad2		: 40;
+		}	index;
+	};
+	__u64		notice_addr;
+};
+
+static struct irde_desc irde_descs[MAX_IR_ENGINES];
+static phys_addr_t msi_base_addr;
+static phys_addr_t redirect_reg_base = 0x1fe00000;
+
+#define REDIRECT_REG_BASE(reg, node) \
+	(UNCACHE_BASE | redirect_reg_base | (u64)(node) << NODE_ADDRSPACE_SHIFT | (reg))
+#define	redirect_reg_queue_head(node)	REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQH, (node))
+#define	redirect_reg_queue_tail(node)	REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQT, (node))
+#define read_queue_head(node)		(*((u32 *)(redirect_reg_queue_head(node))))
+#define read_queue_tail(node)		(*((u32 *)(redirect_reg_queue_tail(node))))
+#define write_queue_tail(node, val)	(*((u32 *)(redirect_reg_queue_tail(node))) = (val))
+
+static inline bool invalid_queue_is_full(int node, u32 *tail)
+{
+	u32 head = read_queue_head(node);
+
+	*tail = read_queue_tail(node);
+
+	return head == ((*tail + 1) % INVALID_QUEUE_SIZE);
+}
+
+static void invalid_enqueue(struct redirect_queue *rqueue, struct irde_inv_cmd *cmd)
+{
+	struct irde_inv_cmd *inv_addr;
+	u32 tail;
+
+	guard(raw_spinlock_irqsave)(&rqueue->lock);
+
+	while (invalid_queue_is_full(rqueue->node, &tail))
+		cpu_relax();
+
+	inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
+	memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));
+	tail = (tail + 1) % INVALID_QUEUE_SIZE;
+
+	/*
+	 * the barrier ensure that the previously written data is visible
+	 * before updating the tail register
+	 */
+	wmb();
+
+	write_queue_tail(rqueue->node, tail);
+}
+
+static void irde_invlid_entry_node(struct redirect_item *item)
+{
+	struct redirect_queue *rqueue;
+	struct irde_inv_cmd cmd;
+	volatile u64 raddr = 0;
+	int node = item->table->node;
+
+	rqueue = &(irde_descs[node].inv_queue);
+	cmd.cmd_info = 0;
+	cmd.index.type = INVALID_INDEX;
+	cmd.index.need_notice = 1;
+	cmd.index.index = item->index;
+	cmd.notice_addr = (u64)(__pa(&raddr));
+
+	invalid_enqueue(rqueue, &cmd);
+
+	while (!raddr)
+		cpu_relax();
+
+}
+
+static inline struct avecintc_data *irq_data_get_avec_data(struct irq_data *data)
+{
+	return data->parent_data->chip_data;
+}
+
+static int redirect_table_alloc(struct redirect_item *item, struct redirect_table *ird_table)
+{
+	int index;
+
+	guard(raw_spinlock_irqsave)(&ird_table->lock);
+
+	index = find_first_zero_bit(ird_table->bitmap, IRD_ENTRIES);
+	if (index > IRD_ENTRIES) {
+		pr_err("No redirect entry to use\n");
+		return -ENOMEM;
+	}
+
+	__set_bit(index, ird_table->bitmap);
+
+	item->index = index;
+	item->entry = &ird_table->table[index];
+	item->table = ird_table;
+
+	return 0;
+}
+
+static void redirect_table_free(struct redirect_item *item)
+{
+	struct redirect_table *ird_table;
+	struct redirect_entry *entry;
+
+	ird_table = item->table;
+
+	entry = item->entry;
+	memset(entry, 0, sizeof(struct redirect_entry));
+
+	scoped_guard(raw_spinlock_irqsave, &ird_table->lock)
+		bitmap_release_region(ird_table->bitmap, item->index, 0);
+
+	kfree(item->gpid);
+
+	irde_invlid_entry_node(item);
+}
+
+static inline void redirect_domain_prepare_entry(struct redirect_item *item,
+						 struct avecintc_data *adata)
+{
+	struct redirect_entry *entry = item->entry;
+
+	item->gpid->en = 1;
+	item->gpid->irqnum = adata->vec;
+	item->gpid->dst = adata->cpu;
+
+	entry->lo.valid = 1;
+	entry->lo.gpid = ((long)item->gpid >> GPID_ADDR_SHIFT) & (GPID_ADDR_MASK);
+	entry->lo.vector = 0xff;
+}
+
+static int redirect_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force)
+{
+	struct redirect_item *item = data->chip_data;
+	struct avecintc_data *adata;
+	int ret;
+
+	ret = irq_chip_set_affinity_parent(data, dest, force);
+	if (ret == IRQ_SET_MASK_OK_DONE) {
+		return ret;
+	} else if (ret) {
+		pr_err("IRDE:set_affinity error %d\n", ret);
+		return ret;
+	}
+
+	adata = irq_data_get_avec_data(data);
+	redirect_domain_prepare_entry(item, adata);
+	irde_invlid_entry_node(item);
+	avecintc_sync(adata);
+
+	return IRQ_SET_MASK_OK;
+}
+
+static void redirect_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct redirect_item *item;
+
+	item = irq_data_get_irq_chip_data(d);
+	msg->address_lo = (msi_base_addr | 1 << 2 | ((item->index & 0xffff) << 4));
+	msg->address_hi = 0x0;
+	msg->data = 0x0;
+}
+
+static struct irq_chip loongarch_redirect_chip = {
+	.name			= "REDIRECT",
+	.irq_ack		= irq_chip_ack_parent,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_set_affinity	= redirect_set_affinity,
+	.irq_compose_msi_msg	= redirect_compose_msi_msg,
+};
+
+static void redirect_free_resources(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs)
+{
+	for (int i = 0; i < nr_irqs; i++) {
+		struct irq_data *irq_data;
+
+		irq_data = irq_domain_get_irq_data(domain, virq  + i);
+		if (irq_data && irq_data->chip_data) {
+			struct redirect_item *item;
+
+			item = irq_data->chip_data;
+			redirect_table_free(item);
+			kfree(item);
+		}
+	}
+}
+
+static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
+			unsigned int nr_irqs, void *arg)
+{
+	struct redirect_table *ird_table;
+	struct avecintc_data *avec_data;
+	struct irq_data *irq_data;
+	msi_alloc_info_t *info;
+	int ret, i, node;
+
+	info = (msi_alloc_info_t *)arg;
+	node = dev_to_node(info->desc->dev);
+	ird_table = &irde_descs[node].ird_table;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++) {
+		struct redirect_item *item;
+
+		item = kzalloc(sizeof(struct redirect_item), GFP_KERNEL);
+		if (!item) {
+			pr_err("Alloc redirect descriptor failed\n");
+			goto out_free_resources;
+		}
+
+		irq_data = irq_domain_get_irq_data(domain, virq + i);
+
+		avec_data = irq_data_get_avec_data(irq_data);
+		ret = redirect_table_alloc(item, ird_table);
+		if (ret) {
+			pr_err("Alloc redirect table entry failed\n");
+			goto out_free_resources;
+		}
+
+		item->gpid = kzalloc_node(sizeof(struct redirect_gpid), GFP_KERNEL, node);
+		if (!item->gpid) {
+			pr_err("Alloc redirect GPID failed\n");
+			goto out_free_resources;
+		}
+
+		irq_data->chip_data = item;
+		irq_data->chip = &loongarch_redirect_chip;
+		redirect_domain_prepare_entry(item, avec_data);
+	}
+	return 0;
+
+out_free_resources:
+	redirect_free_resources(domain, virq, nr_irqs);
+	irq_domain_free_irqs_common(domain, virq, nr_irqs);
+
+	return -ENOMEM;
+}
+
+static void redirect_domain_free(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs)
+{
+	redirect_free_resources(domain, virq, nr_irqs);
+	return irq_domain_free_irqs_common(domain, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops redirect_domain_ops = {
+	.alloc		= redirect_domain_alloc,
+	.free		= redirect_domain_free,
+	.select		= msi_lib_irq_domain_select,
+};
+
+static int redirect_queue_init(int node)
+{
+	struct redirect_queue *rqueue = &(irde_descs[node].inv_queue);
+	struct page *pages;
+
+	pages = alloc_pages_node(0, GFP_KERNEL | __GFP_ZERO, INVALID_QUEUE_PAGE_ORDER);
+	if (!pages) {
+		pr_err("Node [%d] Invalid Queue alloc pages failed!\n", node);
+		return -ENOMEM;
+	}
+
+	rqueue->page = pages;
+	rqueue->base = (u64)page_address(pages);
+	rqueue->max_size = INVALID_QUEUE_SIZE;
+	rqueue->head = 0;
+	rqueue->tail = 0;
+	rqueue->node = node;
+	raw_spin_lock_init(&rqueue->lock);
+
+	iocsr_write32(0, LOONGARCH_IOCSR_REDIRECT_CQH);
+	iocsr_write32(0, LOONGARCH_IOCSR_REDIRECT_CQT);
+	iocsr_write64(((rqueue->base & (CQB_ADDR_MASK << CQB_ADDR_SHIFT)) |
+				(CQB_SIZE_MASK << CQB_SIZE_SHIFT)), LOONGARCH_IOCSR_REDIRECT_CQB);
+	return 0;
+}
+
+static int redirect_table_init(int node)
+{
+	struct redirect_table *ird_table = &(irde_descs[node].ird_table);
+	unsigned long *bitmap;
+	struct page *pages;
+	int ret;
+
+	pages = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, IRD_PAGE_ORDER);
+	if (!pages) {
+		pr_err("Node [%d] redirect table alloc pages failed!\n", node);
+		return -ENOMEM;
+	}
+	ird_table->page = pages;
+	ird_table->table = page_address(pages);
+
+	bitmap = bitmap_zalloc(IRD_ENTRIES, GFP_KERNEL);
+	if (!bitmap) {
+		pr_err("Node [%d] redirect table bitmap alloc pages failed!\n", node);
+		ret = -ENOMEM;
+		goto free_pages;
+	}
+
+	ird_table->bitmap = bitmap;
+	ird_table->nr_ird = IRD_ENTRIES;
+	ird_table->node = node;
+
+	raw_spin_lock_init(&ird_table->lock);
+
+	if (redirect_queue_init(node)) {
+		ret = -EINVAL;
+		goto free_bitmap;
+	}
+
+	iocsr_write64(CFG_DISABLE_IDLE, LOONGARCH_IOCSR_REDIRECT_CFG);
+	iocsr_write64(__pa(ird_table->table), LOONGARCH_IOCSR_REDIRECT_TBR);
+
+	return 0;
+
+free_pages:
+	__free_pages(pages, IRD_PAGE_ORDER);
+free_bitmap:
+	bitmap_free(bitmap);
+	return ret;
+}
+
+static void redirect_table_fini(int node)
+{
+	struct redirect_table *ird_table = &(irde_descs[node].ird_table);
+	struct redirect_queue *rqueue = &(irde_descs[node].inv_queue);
+
+	if (ird_table->page) {
+		__free_pages(ird_table->page, IRD_PAGE_ORDER);
+		ird_table->table = NULL;
+		ird_table->page = NULL;
+	}
+
+	if (ird_table->page) {
+		bitmap_free(ird_table->bitmap);
+		ird_table->bitmap = NULL;
+	}
+
+	if (rqueue->page) {
+		__free_pages(rqueue->page, INVALID_QUEUE_PAGE_ORDER);
+		rqueue->page = NULL;
+		rqueue->base = 0;
+	}
+
+	iocsr_write64(0, LOONGARCH_IOCSR_REDIRECT_CQB);
+	iocsr_write64(0, LOONGARCH_IOCSR_REDIRECT_TBR);
+}
+
+static int redirect_cpu_online(unsigned int cpu)
+{
+	int ret, node = cpu_to_node(cpu);
+
+	if (cpu != cpumask_first(cpumask_of_node(node)))
+		return 0;
+
+	if (irde_descs[node].finish)
+		return 0;
+
+	ret = redirect_table_init(node);
+	if (ret) {
+		redirect_table_fini(node);
+		return -EINVAL;
+	}
+
+	irde_descs[node].finish = 1;
+	return 0;
+}
+
+#ifdef	CONFIG_ACPI
+static int __init redirect_reg_base_init(void)
+{
+	acpi_status status;
+	uint64_t addr;
+
+	if (acpi_disabled)
+		return 0;
+
+	status = acpi_evaluate_integer(NULL, "\\_SB.NO00", NULL, &addr);
+	if (ACPI_FAILURE(status))
+		pr_info("redirect_iocsr_base used default 0x1fe00000\n");
+	else
+		redirect_reg_base = addr;
+
+	return 0;
+}
+subsys_initcall_sync(redirect_reg_base_init);
+
+static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
+		const unsigned long end)
+{
+	struct acpi_madt_msi_pic *pchmsi_entry = (struct acpi_madt_msi_pic *)header;
+
+	msi_base_addr = pchmsi_entry->msg_address - AVEC_MSG_OFFSET;
+
+	return pch_msi_acpi_init_avec(redirect_domain);
+}
+
+static int __init acpi_cascade_irqdomain_init(void)
+{
+	return acpi_table_parse_madt(ACPI_MADT_TYPE_MSI_PIC, pch_msi_parse_madt, 1);
+}
+
+int __init redirect_acpi_init(struct irq_domain *parent)
+{
+	struct fwnode_handle *fwnode;
+	struct irq_domain *domain;
+	int ret;
+
+	fwnode = irq_domain_alloc_named_fwnode("redirect");
+	if (!fwnode) {
+		pr_err("Unable to alloc redirect domain handle\n");
+		goto fail;
+	}
+
+	domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
+					     &redirect_domain_ops, irde_descs);
+	if (!domain) {
+		pr_err("Unable to alloc redirect domain\n");
+		goto out_free_fwnode;
+	}
+
+	redirect_domain = domain;
+
+	ret = redirect_table_init(0);
+	if (ret)
+		goto out_free_table;
+
+	ret = acpi_cascade_irqdomain_init();
+	if (ret < 0) {
+		pr_err("Failed to cascade IRQ domain, ret=%d\n", ret);
+		goto out_free_table;
+	}
+
+	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_REDIRECT_STARTING,
+				  "irqchip/loongarch/redirect:starting",
+				  redirect_cpu_online, NULL);
+
+	pr_info("loongarch irq redirect modules init succeeded\n");
+	return 0;
+
+out_free_table:
+	redirect_table_fini(0);
+	irq_domain_remove(redirect_domain);
+	redirect_domain = NULL;
+out_free_fwnode:
+	irq_domain_free_fwnode(fwnode);
+fail:
+	return -EINVAL;
+}
+#endif
diff --git a/drivers/irqchip/irq-loongson.h b/drivers/irqchip/irq-loongson.h
index 11fa138d1f44..05ad40ffb62b 100644
--- a/drivers/irqchip/irq-loongson.h
+++ b/drivers/irqchip/irq-loongson.h
@@ -5,6 +5,15 @@
 
 #ifndef _DRIVERS_IRQCHIP_IRQ_LOONGSON_H
 #define _DRIVERS_IRQCHIP_IRQ_LOONGSON_H
+#define AVEC_MSG_OFFSET		0x100000
+struct avecintc_data {
+	struct list_head        entry;
+	unsigned int            cpu;
+	unsigned int            vec;
+	unsigned int            prev_cpu;
+	unsigned int            prev_vec;
+	unsigned int            moving;
+};
 
 int find_pch_pic(u32 gsi);
 
@@ -24,4 +33,7 @@ int pch_msi_acpi_init(struct irq_domain *parent,
 					struct acpi_madt_msi_pic *acpi_pchmsi);
 int pch_msi_acpi_init_avec(struct irq_domain *parent);
 
+int redirect_acpi_init(struct irq_domain *parent);
+
+void avecintc_sync(struct avecintc_data *adata);
 #endif /* _DRIVERS_IRQCHIP_IRQ_LOONGSON_H */
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 1987400000b4..6a4ff072db42 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -145,6 +145,7 @@ enum cpuhp_state {
 	CPUHP_AP_IRQ_MIPS_GIC_STARTING,
 	CPUHP_AP_IRQ_EIOINTC_STARTING,
 	CPUHP_AP_IRQ_AVECINTC_STARTING,
+	CPUHP_AP_IRQ_REDIRECT_STARTING,
 	CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
 	CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
 	CPUHP_AP_IRQ_RISCV_IMSIC_STARTING,
-- 
2.41.0

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Thomas Gleixner]

On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
>  		if (cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
> -			return 0;
> +			/*
> +			 * The new affinity configuration has taken effect,
> +			 * and returning IRQ_SET_MASK_OK_DONE here indicates that no further
> +			 * changes need to be made in the subsequent process

This is not what IRQ_SET_MASK_OK_DONE is about. The documentation
clearly says:

 * IRQ_SET_MASK_OK_DONE - Same as IRQ_SET_MASK_OK for core. Special code to
 *                        support stacked irqchips, which indicates skipping
 *                        all descendant irqchips.

It's not about further changes. It's about preventing invoking
set_affinity() callbacks down the hierarchy.

And you still fail to explain why this change is correct for the
existing code. That explanation wants to be in the changelog of the
seperate patch doing this change.

And then you can spare the comment, which is btw. also violating the
bracket rules in the tip maintainers documentation.


> +			 */
> +			return IRQ_SET_MASK_OK_DONE;
>  
>  		cpumask_and(&intersect_mask, dest, cpu_online_mask);
>  
> @@ -121,7 +116,8 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
>  		adata->cpu = cpu;
>  		adata->vec = vector;
>  		per_cpu_ptr(irq_map, adata->cpu)[adata->vec] = irq_data_to_desc(data);
> -		avecintc_sync(adata);
> +		if (!cpu_has_redirectint)
> +			avecintc_sync(adata);
>  	}
>  
>  	irq_data_update_effective_affinity(data, cpumask_of(cpu));
> @@ -412,6 +408,9 @@ static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
>  
>  static inline int __init acpi_cascade_irqdomain_init(void)
>  {
> +	if (cpu_has_redirectint)
> +		return redirect_acpi_init(loongarch_avec.domain);
> +
>  	return acpi_table_parse_madt(ACPI_MADT_TYPE_MSI_PIC, pch_msi_parse_madt, 1);
>  }
>  
> diff --git a/drivers/irqchip/irq-loongarch-ir.c b/drivers/irqchip/irq-loongarch-ir.c
> new file mode 100644
> index 000000000000..ae42ec5028d2
> --- /dev/null
> +++ b/drivers/irqchip/irq-loongarch-ir.c
> @@ -0,0 +1,562 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Loongson Technologies, Inc.
> + */
> +
> +#include <linux/cpuhotplug.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/spinlock.h>
> +#include <linux/msi.h>
> +
> +#include <asm/irq.h>
> +#include <asm/loongarch.h>
> +#include <asm/setup.h>
> +#include <larchintrin.h>
> +
> +#include "irq-loongson.h"
> +#include "irq-msi-lib.h"
> +
> +#define IRD_ENTRIES			65536
> +
> +/* redirect entry size 128bits */
> +#define IRD_PAGE_ORDER			(20 - PAGE_SHIFT)
> +
> +/* irt cache invalid queue */
> +#define	INVALID_QUEUE_SIZE		4096

Use SPACE after #define, not TAB. All over the place...

> +#define INVALID_QUEUE_PAGE_ORDER	(16 - PAGE_SHIFT)

I'm pretty sure that the above magic numbers have dependencies in some
way, right? So why is it not expressed in a way which makes this obvious? 

These magic numbers are just incomprehensible as they make the reader
guess what this is about. Here is my (probably not so) wild guess:

#define IRD_ENTRY_SIZE			16

#define IRD_ENTRIES			65536
#define IRD_PAGE_ORDER			get_order(IRD_ENTRIES * IRD_ENTRY_SIZE)

#define	INVALID_QUEUE_SIZE		4096
#define IRD_INVALID__QUEUE_PAGE_ORDER	get_order(INVALID_QUEUE_SIZE * IRD_ENTRY_SIZE)

No?

> +#define GPID_ADDR_MASK			0x3ffffffffffULL

GENMASK()

> +#define GPID_ADDR_SHIFT			6
> +
> +#define CQB_SIZE_SHIFT			0
> +#define CQB_SIZE_MASK			0xf
> +#define CQB_ADDR_SHIFT			12
> +#define CQB_ADDR_MASK			(0xfffffffffULL)

GENMASK()

> +#define CFG_DISABLE_IDLE		2
> +#define INVALID_INDEX			0
> +
> +#define MAX_IR_ENGINES			16


> +struct redirect_gpid {
> +	u64	pir[4];      /* Pending interrupt requested */
> +	u8	en	: 1, /* doorbell */

Use C++ style tail comments for this as documented. Do you I really have
to point out every single item in the documentation explicitely or can
you just read all of it and follow the guidelines?

> +struct irde_desc {
> +	struct	redirect_table	ird_table;
> +	struct	redirect_queue	inv_queue;
> +	int	finish;

Groan.

"Struct declarations should align the struct member names in a tabular fashion:

 struct bar_order {
        unsigned int    guest_id;
        int             ordered_item;
        struct menu     *menu;
 };"

It clearly says to align the struct member names, no?

Otherwise the example would be:

 struct bar_order {
        unsigned int    guest_id;
        int             ordered_item;
        struct 		menu     	*menu;
 };

which is unreadable garbage obviously.

> +};

> +static void invalid_enqueue(struct redirect_queue *rqueue, struct irde_inv_cmd *cmd)
> +{
> +	struct irde_inv_cmd *inv_addr;
> +	u32 tail;
> +
> +	guard(raw_spinlock_irqsave)(&rqueue->lock);
> +
> +	while (invalid_queue_is_full(rqueue->node, &tail))
> +		cpu_relax();
> +
> +	inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
> +	memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));

Seriously?

struct redirect_queue {
	int				node;
        union {
 		u64			base;
                struct irde_inv_cmd	*cmds;
        };
	u32				max_size;
        ...
};

and then this becomes

    	memcpy(&rqueue->cmds[tail], cmd, sizeof(cmd));

That's too comprehensible, right?

> +	tail = (tail + 1) % INVALID_QUEUE_SIZE;

Why is this before the barrier? The barrier does not do anything about
this and you can simplify this. See below.

> +	/*
> +	 * the barrier ensure that the previously written data is visible

This barrier ensures .....

> +	 * before updating the tail register

And as there is no rmb() counterpart you want to explain that this is
serializing against the hardware.

> +	 */
> +	wmb();
> +
> +	write_queue_tail(rqueue->node, tail);

	write_queue_tail(rqueue->node, (tail + 1) & INVALID_QUEUE_MASK);

No?

> +}
> +
> +static void irde_invlid_entry_node(struct redirect_item *item)
> +{
> +	struct redirect_queue *rqueue;
> +	struct irde_inv_cmd cmd;
> +	volatile u64 raddr = 0;

No. See Documentation/process/volatile-considered-harmful.rst

> +static void redirect_table_free(struct redirect_item *item)
> +{
> +	struct redirect_table *ird_table;
> +	struct redirect_entry *entry;
> +
> +	ird_table = item->table;
> +
> +	entry = item->entry;
> +	memset(entry, 0, sizeof(struct redirect_entry));

        memset(entry, 0, sizeof(entry));

It's obvious why using sizeof(type) is a bad idea.

> +	scoped_guard(raw_spinlock_irqsave, &ird_table->lock)

raw_spinlock_irq as this code can never be invoked from an interrupt
disabled region.

> +		bitmap_release_region(ird_table->bitmap, item->index, 0);
> +
> +	kfree(item->gpid);
> +
> +	irde_invlid_entry_node(item);
> +}

> +static inline void redirect_domain_prepare_entry(struct redirect_item *item,
> +						 struct avecintc_data *adata)
> +{
> +	struct redirect_entry *entry = item->entry;
> +
> +	item->gpid->en = 1;
> +	item->gpid->irqnum = adata->vec;
> +	item->gpid->dst = adata->cpu;
> +
> +	entry->lo.valid = 1;
> +	entry->lo.gpid = ((long)item->gpid >> GPID_ADDR_SHIFT) & (GPID_ADDR_MASK);

Hardware addresses are type unsigned long and not long.

> +	entry->lo.vector = 0xff;
> +}

> +static void redirect_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> +	struct redirect_item *item;
> +
> +	item = irq_data_get_irq_chip_data(d);

Just move the initialization into the declaration line.

> +	msg->address_lo = (msi_base_addr | 1 << 2 | ((item->index & 0xffff) << 4));
> +	msg->address_hi = 0x0;
> +	msg->data = 0x0;
> +}

> +static void redirect_free_resources(struct irq_domain *domain, unsigned int virq,
> +				    unsigned int nr_irqs)
> +{
> +	for (int i = 0; i < nr_irqs; i++) {
> +		struct irq_data *irq_data;
> +
> +		irq_data = irq_domain_get_irq_data(domain, virq  + i);

Ditto.

> +		if (irq_data && irq_data->chip_data) {
> +			struct redirect_item *item;
> +
> +			item = irq_data->chip_data;

Same and all over the place.

> +			redirect_table_free(item);
> +			kfree(item);
> +		}
> +	}
> +}
> +
> +static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +			unsigned int nr_irqs, void *arg)

I asked you before to align the arguments of the second line properly
and according to documentation..

> +{
> +	struct redirect_table *ird_table;
> +	struct avecintc_data *avec_data;
> +	struct irq_data *irq_data;
> +	msi_alloc_info_t *info;
> +	int ret, i, node;
> +
> +	info = (msi_alloc_info_t *)arg;

What's that type cast for?

> +	node = dev_to_node(info->desc->dev);
> +	ird_table = &irde_descs[node].ird_table;
> +
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		struct redirect_item *item;
> +
> +		item = kzalloc(sizeof(struct redirect_item), GFP_KERNEL);
> +		if (!item) {
> +			pr_err("Alloc redirect descriptor failed\n");
> +			goto out_free_resources;
> +		}
> +
> +		irq_data = irq_domain_get_irq_data(domain, virq + i);
> +
> +		avec_data = irq_data_get_avec_data(irq_data);

Neither irq_data nor avec_data are used here and only required way
down. Can you structure your code so it makes sense?

> +		ret = redirect_table_alloc(item, ird_table);
> +		if (ret) {
> +			pr_err("Alloc redirect table entry failed\n");
> +			goto out_free_resources;
> +		}
> +
> +		item->gpid = kzalloc_node(sizeof(struct redirect_gpid), GFP_KERNEL, node);
> +		if (!item->gpid) {
> +			pr_err("Alloc redirect GPID failed\n");
> +			goto out_free_resources;
> +		}

Why do you need this extra allocation here instead of simply embedding
gpid into item?

> +		irq_data->chip_data = item;
> +		irq_data->chip = &loongarch_redirect_chip;
> +		redirect_domain_prepare_entry(item, avec_data);
> +	}
> +	return 0;

> +	iocsr_write64(((rqueue->base & (CQB_ADDR_MASK << CQB_ADDR_SHIFT)) |
> +				(CQB_SIZE_MASK << CQB_SIZE_SHIFT)), LOONGARCH_IOCSR_REDIRECT_CQB);

Align second line properly.

> +	return 0;
> +}
> +
> +static int redirect_table_init(int node)
> +{
> +	struct redirect_table *ird_table = &(irde_descs[node].ird_table);

Remove the pointless brackets.

> +	unsigned long *bitmap;
> +	struct page *pages;
> +	int ret;
> +
> +	pages = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, IRD_PAGE_ORDER);
> +	if (!pages) {
> +		pr_err("Node [%d] redirect table alloc pages failed!\n", node);
> +		return -ENOMEM;
> +	}
> +	ird_table->page = pages;
> +	ird_table->table = page_address(pages);
> +
> +	bitmap = bitmap_zalloc(IRD_ENTRIES, GFP_KERNEL);
> +	if (!bitmap) {
> +		pr_err("Node [%d] redirect table bitmap alloc pages failed!\n", node);
> +		ret = -ENOMEM;
> +		goto free_pages;
> +	}
> +
> +	ird_table->bitmap = bitmap;
> +	ird_table->nr_ird = IRD_ENTRIES;
> +	ird_table->node = node;
> +
> +	raw_spin_lock_init(&ird_table->lock);
> +
> +	if (redirect_queue_init(node)) {
> +		ret = -EINVAL;
> +		goto free_bitmap;

So redirect_queue_init() returns -ENOMEM which is then converted to
-EINVAL here. That makes absolutely no sense at all.

Neither makes the 'ret' variable sense because all failures should
return -ENOMEM and therefore you can make redirect_queue_init() return
bool (true on success) and return -ENOMEM in the failure path.

No?

> +static void redirect_table_fini(int node)
> +{
> +	struct redirect_table *ird_table = &(irde_descs[node].ird_table);
> +	struct redirect_queue *rqueue = &(irde_descs[node].inv_queue);

No brackets. They have no value and just disturb reading.

> +static int redirect_cpu_online(unsigned int cpu)
> +{
> +	int ret, node = cpu_to_node(cpu);
> +
> +	if (cpu != cpumask_first(cpumask_of_node(node)))
> +		return 0;
> +
> +	if (irde_descs[node].finish)
> +		return 0;

What's this finish thing about?

Neither the CPU mask check nor this finish hack is required:

        if (irde_descs[node].pages)
        	return 0;

covers all of it, no?

> +	ret = redirect_table_init(node);
> +	if (ret) {
> +		redirect_table_fini(node);

What is this for? You already mopped up the mess in the failure path of
redirect_table_init(), so doing it again makes no sense.

Just get rid of the failure path in redirect_table_init() and let that
return a bool (true on success) and invoke redirect_table_fini() here

> +		return -EINVAL;

Seriously? The failure condition is -ENOMEM so why do you want to change
that?

> +	}
> +
> +	irde_descs[node].finish = 1;
> +	return 0;
> +}
> +
> +#ifdef	CONFIG_ACPI

What's the TAB for?

> +static int __init redirect_reg_base_init(void)
> +{
> +	acpi_status status;
> +	uint64_t addr;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	status = acpi_evaluate_integer(NULL, "\\_SB.NO00", NULL, &addr);
> +	if (ACPI_FAILURE(status))
> +		pr_info("redirect_iocsr_base used default 0x1fe00000\n");
> +	else
> +		redirect_reg_base = addr;
> +
> +	return 0;
> +}
> +subsys_initcall_sync(redirect_reg_base_init);
> +
> +static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
> +		const unsigned long end)

Sigh.

> +int __init redirect_acpi_init(struct irq_domain *parent)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct irq_domain *domain;
> +	int ret;
> +
> +	fwnode = irq_domain_alloc_named_fwnode("redirect");
> +	if (!fwnode) {
> +		pr_err("Unable to alloc redirect domain handle\n");
> +		goto fail;
> +	}
> +
> +	domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
> +					     &redirect_domain_ops, irde_descs);
> +	if (!domain) {
> +		pr_err("Unable to alloc redirect domain\n");
> +		goto out_free_fwnode;
> +	}
> +
> +	redirect_domain = domain;

What's the point of this local domain variable?

> +	ret = redirect_table_init(0);
> +	if (ret)
> +		goto out_free_table;
> +

Oh well...

Thanks,

        tglx

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Tianyang Zhang]

Hi, Thomas

在 2025/6/13 下午11:20, Thomas Gleixner 写道:
> On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
>>   		if (cpu_online(adata->cpu) && cpumask_test_cpu(adata->cpu, dest))
>> -			return 0;
>> +			/*
>> +			 * The new affinity configuration has taken effect,
>> +			 * and returning IRQ_SET_MASK_OK_DONE here indicates that no further
>> +			 * changes need to be made in the subsequent process
> This is not what IRQ_SET_MASK_OK_DONE is about. The documentation
> clearly says:
>
>   * IRQ_SET_MASK_OK_DONE - Same as IRQ_SET_MASK_OK for core. Special code to
>   *                        support stacked irqchips, which indicates skipping
>   *                        all descendant irqchips.
>
> It's not about further changes. It's about preventing invoking
> set_affinity() callbacks down the hierarchy.
>
> And you still fail to explain why this change is correct for the
> existing code. That explanation wants to be in the changelog of the
> seperate patch doing this change.
>
> And then you can spare the comment, which is btw. also violating the
> bracket rules in the tip maintainers documentation.

OK, This means that this change involves adjustments to the code 
organization

and cannot be included solely in a large patch. It requires clearer 
submission records and explanations

If that's the case, I will make the necessary modifications as 
requested. Thank you

>
>
>> +			 */
>> +			return IRQ_SET_MASK_OK_DONE;
>>   
>>   		cpumask_and(&intersect_mask, dest, cpu_online_mask);
>>   
>> @@ -121,7 +116,8 @@ static int avecintc_set_affinity(struct irq_data *data, const struct cpumask *de
>>   		adata->cpu = cpu;
>>   		adata->vec = vector;
>>   		per_cpu_ptr(irq_map, adata->cpu)[adata->vec] = irq_data_to_desc(data);
>> -		avecintc_sync(adata);
>> +		if (!cpu_has_redirectint)
>> +			avecintc_sync(adata);
>>   	}
>>   
>>   	irq_data_update_effective_affinity(data, cpumask_of(cpu));
>> @@ -412,6 +408,9 @@ static int __init pch_msi_parse_madt(union acpi_subtable_headers *header,
>>   
>>   static inline int __init acpi_cascade_irqdomain_init(void)
>>   {
>> +	if (cpu_has_redirectint)
>> +		return redirect_acpi_init(loongarch_avec.domain);
>> +
>>   	return acpi_table_parse_madt(ACPI_MADT_TYPE_MSI_PIC, pch_msi_parse_madt, 1);
>>   }
>>   
>> diff --git a/drivers/irqchip/irq-loongarch-ir.c b/drivers/irqchip/irq-loongarch-ir.c
>> new file mode 100644
>> index 000000000000..ae42ec5028d2
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-loongarch-ir.c
>> @@ -0,0 +1,562 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Loongson Technologies, Inc.
>> + */
>> +
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/msi.h>
>> +
>> +#include <asm/irq.h>
>> +#include <asm/loongarch.h>
>> +#include <asm/setup.h>
>> +#include <larchintrin.h>
>> +
>> +#include "irq-loongson.h"
>> +#include "irq-msi-lib.h"
>> +
>> +#define IRD_ENTRIES			65536
>> +
>> +/* redirect entry size 128bits */
>> +#define IRD_PAGE_ORDER			(20 - PAGE_SHIFT)
>> +
>> +/* irt cache invalid queue */
>> +#define	INVALID_QUEUE_SIZE		4096
> Use SPACE after #define, not TAB. All over the place...
OK, I got it
>
>> +#define INVALID_QUEUE_PAGE_ORDER	(16 - PAGE_SHIFT)
> I'm pretty sure that the above magic numbers have dependencies in some
> way, right? So why is it not expressed in a way which makes this obvious?
>
> These magic numbers are just incomprehensible as they make the reader
> guess what this is about. Here is my (probably not so) wild guess:
>
> #define IRD_ENTRY_SIZE			16
>
> #define IRD_ENTRIES			65536
> #define IRD_PAGE_ORDER			get_order(IRD_ENTRIES * IRD_ENTRY_SIZE)
>
> #define	INVALID_QUEUE_SIZE		4096
> #define IRD_INVALID__QUEUE_PAGE_ORDER	get_order(INVALID_QUEUE_SIZE * IRD_ENTRY_SIZE)
>
> No?
Yes, this is better way , and I will pay attention to improving the 
readability of the code in the future
>
> GENMASK()
Ok , I got it , thanks
>
>> +#define CFG_DISABLE_IDLE		2
>> +#define INVALID_INDEX			0
>> +
>> +#define MAX_IR_ENGINES			16
>
>> +struct redirect_gpid {
>> +	u64	pir[4];      /* Pending interrupt requested */
>> +	u8	en	: 1, /* doorbell */
> Use C++ style tail comments for this as documented. Do you I really have
> to point out every single item in the documentation explicitely or can
> you just read all of it and follow the guidelines?

Okay, here are some references to existing code....

I will reorganize all code style issues according to the documentation

>
>> +struct irde_desc {
>> +	struct	redirect_table	ird_table;
>> +	struct	redirect_queue	inv_queue;
>> +	int	finish;
> Groan.
>
> "Struct declarations should align the struct member names in a tabular fashion:
>
>   struct bar_order {
>          unsigned int    guest_id;
>          int             ordered_item;
>          struct menu     *menu;
>   };"
>
> It clearly says to align the struct member names, no?
>
> Otherwise the example would be:
>
>   struct bar_order {
>          unsigned int    guest_id;
>          int             ordered_item;
>          struct 		menu     	*menu;
>   };
>
> which is unreadable garbage obviously.
Ok, I will adjust here and check for other similar issues
>
>> +static void invalid_enqueue(struct redirect_queue *rqueue, struct irde_inv_cmd *cmd)
>> +{
>> +	struct irde_inv_cmd *inv_addr;
>> +	u32 tail;
>> +
>> +	guard(raw_spinlock_irqsave)(&rqueue->lock);
>> +
>> +	while (invalid_queue_is_full(rqueue->node, &tail))
>> +		cpu_relax();
>> +
>> +	inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
>> +	memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));
> Seriously?
>
> struct redirect_queue {
> 	int				node;
>          union {
>   		u64			base;
>                  struct irde_inv_cmd	*cmds;
>          };
> 	u32				max_size;
>          ...
> };
>
> and then this becomes
>
>      	memcpy(&rqueue->cmds[tail], cmd, sizeof(cmd));
>
> That's too comprehensible, right?
Yes, this is a more readable writing style, thank you
>
>> +	tail = (tail + 1) % INVALID_QUEUE_SIZE;
> Why is this before the barrier? The barrier does not do anything about
> this and you can simplify this. See below.
>
> And as there is no rmb() counterpart you want to explain that this is
> serializing against the hardware.
>> +	 */
>> +	wmb();
>> +
>> +	write_queue_tail(rqueue->node, tail);
> 	write_queue_tail(rqueue->node, (tail + 1) & INVALID_QUEUE_MASK);
>
> No?

The reason fo coding here is that during testing, it was found that a 
barrier is needed between the

update of temporary variable 'tail' and the operation of register with 
'write_queue_tail'

, otherwise write_queue_tail will probabilistically fail to obtain the 
correct value.

We will conduct testing on the above methods, and if possible, make 
changes as required, or promote improvements in code readability

thanks

>
>> +}
>> +
>> +static void irde_invlid_entry_node(struct redirect_item *item)
>> +{
>> +	struct redirect_queue *rqueue;
>> +	struct irde_inv_cmd cmd;
>> +	volatile u64 raddr = 0;
> No. See Documentation/process/volatile-considered-harmful.rst
Ok, I will follow the agreement here
>
>> +static void redirect_table_free(struct redirect_item *item)
>> +{
>> +	struct redirect_table *ird_table;
>> +	struct redirect_entry *entry;
>> +
>> +	ird_table = item->table;
>> +
>> +	entry = item->entry;
>> +	memset(entry, 0, sizeof(struct redirect_entry));
>          memset(entry, 0, sizeof(entry));
>
> It's obvious why using sizeof(type) is a bad idea.
Ok, I got it
>
>> +	scoped_guard(raw_spinlock_irqsave, &ird_table->lock)
> raw_spinlock_irq as this code can never be invoked from an interrupt
> disabled region.
Ok, I will make the necessary corrections here
>
>> +		bitmap_release_region(ird_table->bitmap, item->index, 0);
>> +
>> +	kfree(item->gpid);
>> +
>> +	irde_invlid_entry_node(item);
>> +}
>> +static inline void redirect_domain_prepare_entry(struct redirect_item *item,
>> +						 struct avecintc_data *adata)
>> +{
>> +	struct redirect_entry *entry = item->entry;
>> +
>> +	item->gpid->en = 1;
>> +	item->gpid->irqnum = adata->vec;
>> +	item->gpid->dst = adata->cpu;
>> +
>> +	entry->lo.valid = 1;
>> +	entry->lo.gpid = ((long)item->gpid >> GPID_ADDR_SHIFT) & (GPID_ADDR_MASK);
> Hardware addresses are type unsigned long and not long.
Ok , I got it
>
>> +	entry->lo.vector = 0xff;
>> +}
>> +static void redirect_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
>> +{
>> +	struct redirect_item *item;
>> +
>> +	item = irq_data_get_irq_chip_data(d);
> Just move the initialization into the declaration line.
Ok, I got it
>
>> +		if (irq_data && irq_data->chip_data) {
>> +			struct redirect_item *item;
>> +
>> +			item = irq_data->chip_data;
> Same and all over the place.
>
>> +			redirect_table_free(item);
>> +			kfree(item);
>> +		}
>> +	}
>> +}
>> +
>> +static int redirect_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +			unsigned int nr_irqs, void *arg)
> I asked you before to align the arguments of the second line properly
> and according to documentation..

Sorry, I have reconfirmed the modification records and found that it was 
fixed in the first temporary version.

However, the modification was lost during several testing environment 
migrations in the middle.

I will adjust my code organization process

>
>> +{
>> +	struct redirect_table *ird_table;
>> +	struct avecintc_data *avec_data;
>> +	struct irq_data *irq_data;
>> +	msi_alloc_info_t *info;
>> +	int ret, i, node;
>> +
>> +	info = (msi_alloc_info_t *)arg;
> What's that type cast for?
emm......, An oversight during the code modification process
>
>> +	node = dev_to_node(info->desc->dev);
>> +	ird_table = &irde_descs[node].ird_table;
>> +
>> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		struct redirect_item *item;
>> +
>> +		item = kzalloc(sizeof(struct redirect_item), GFP_KERNEL);
>> +		if (!item) {
>> +			pr_err("Alloc redirect descriptor failed\n");
>> +			goto out_free_resources;
>> +		}
>> +
>> +		irq_data = irq_domain_get_irq_data(domain, virq + i);
>> +
>> +		avec_data = irq_data_get_avec_data(irq_data);
> Neither irq_data nor avec_data are used here and only required way
> down. Can you structure your code so it makes sense?
Ok, I will follow your advice
>
>> +		ret = redirect_table_alloc(item, ird_table);
>> +		if (ret) {
>> +			pr_err("Alloc redirect table entry failed\n");
>> +			goto out_free_resources;
>> +		}
>> +
>> +		item->gpid = kzalloc_node(sizeof(struct redirect_gpid), GFP_KERNEL, node);
>> +		if (!item->gpid) {
>> +			pr_err("Alloc redirect GPID failed\n");
>> +			goto out_free_resources;
>> +		}
> Why do you need this extra allocation here instead of simply embedding
> gpid into item?
Ok, I got it , thanks
>> +		irq_data->chip_data = item;
>> +		irq_data->chip = &loongarch_redirect_chip;
>> +		redirect_domain_prepare_entry(item, avec_data);
>> +	}
>> +	return 0;
>> +	iocsr_write64(((rqueue->base & (CQB_ADDR_MASK << CQB_ADDR_SHIFT)) |
>> +				(CQB_SIZE_MASK << CQB_SIZE_SHIFT)), LOONGARCH_IOCSR_REDIRECT_CQB);
> Align second line properly.
Ok, I got it , thanks
>
>> +	return 0;
>> +}
>> +
>> +static int redirect_table_init(int node)
>> +{
>> +	struct redirect_table *ird_table = &(irde_descs[node].ird_table);
> Remove the pointless brackets.
Ok, I got it , thanks
>
>> +	unsigned long *bitmap;
>> +	struct page *pages;
>> +	int ret;
>> +
>> +	pages = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, IRD_PAGE_ORDER);
>> +	if (!pages) {
>> +		pr_err("Node [%d] redirect table alloc pages failed!\n", node);
>> +		return -ENOMEM;
>> +	}
>> +	ird_table->page = pages;
>> +	ird_table->table = page_address(pages);
>> +
>> +	bitmap = bitmap_zalloc(IRD_ENTRIES, GFP_KERNEL);
>> +	if (!bitmap) {
>> +		pr_err("Node [%d] redirect table bitmap alloc pages failed!\n", node);
>> +		ret = -ENOMEM;
>> +		goto free_pages;
>> +	}
>> +
>> +	ird_table->bitmap = bitmap;
>> +	ird_table->nr_ird = IRD_ENTRIES;
>> +	ird_table->node = node;
>> +
>> +	raw_spin_lock_init(&ird_table->lock);
>> +
>> +	if (redirect_queue_init(node)) {
>> +		ret = -EINVAL;
>> +		goto free_bitmap;
> So redirect_queue_init() returns -ENOMEM which is then converted to
> -EINVAL here. That makes absolutely no sense at all.
>
> Neither makes the 'ret' variable sense because all failures should
> return -ENOMEM and therefore you can make redirect_queue_init() return
> bool (true on success) and return -ENOMEM in the failure path.
>
> No?
Yes, it is a more reasonable way to do this
>> +static void redirect_table_fini(int node)
>> +{
>> +	struct redirect_table *ird_table = &(irde_descs[node].ird_table);
>> +	struct redirect_queue *rqueue = &(irde_descs[node].inv_queue);
> No brackets. They have no value and just disturb reading.
Ok, I got it , thanks
>
>> +static int redirect_cpu_online(unsigned int cpu)
>> +{
>> +	int ret, node = cpu_to_node(cpu);
>> +
>> +	if (cpu != cpumask_first(cpumask_of_node(node)))
>> +		return 0;
>> +
>> +	if (irde_descs[node].finish)
>> +		return 0;
> What's this finish thing about?
>
> Neither the CPU mask check nor this finish hack is required:
>
>          if (irde_descs[node].pages)
>          	return 0;
>
> covers all of it, no?
Ok, I got it , thanks
>
>> +	ret = redirect_table_init(node);
>> +	if (ret) {
>> +		redirect_table_fini(node);
> What is this for? You already mopped up the mess in the failure path of
> redirect_table_init(), so doing it again makes no sense.
>
> Just get rid of the failure path in redirect_table_init() and let that
> return a bool (true on success) and invoke redirect_table_fini() here
There may have been a logical confusion in the version iteration here, 
and I will reorganize this part of the code, thanks
>
>> +		return -EINVAL;
> Seriously? The failure condition is -ENOMEM so why do you want to change
> that?
Ok , return ret is enought there, thanks
>> +	}
>> +
>> +	irde_descs[node].finish = 1;
>> +	return 0;
>> +}
>> +
>> +#ifdef	CONFIG_ACPI
> What's the TAB for?
Ok. I got it , thanks
>> +int __init redirect_acpi_init(struct irq_domain *parent)
>> +{
>> +	struct fwnode_handle *fwnode;
>> +	struct irq_domain *domain;
>> +	int ret;
>> +
>> +	fwnode = irq_domain_alloc_named_fwnode("redirect");
>> +	if (!fwnode) {
>> +		pr_err("Unable to alloc redirect domain handle\n");
>> +		goto fail;
>> +	}
>> +
>> +	domain = irq_domain_create_hierarchy(parent, 0, IRD_ENTRIES, fwnode,
>> +					     &redirect_domain_ops, irde_descs);
>> +	if (!domain) {
>> +		pr_err("Unable to alloc redirect domain\n");
>> +		goto out_free_fwnode;
>> +	}
>> +
>> +	redirect_domain = domain;
> What's the point of this local domain variable?
This is indeed meaningless, thanks
>
>> +	ret = redirect_table_init(0);
>> +	if (ret)
>> +		goto out_free_table;
>> +
> Oh well...
There may have been a logical confusion in the version iteration here, 
and I will reorganize this part of the code, thanks
>
> Thanks,
>
>          tglx

Thanks

Tianyang

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Thomas Gleixner]

On Mon, Jun 23 2025 at 10:45, Tianyang Zhang wrote:
> 在 2025/6/13 下午11:20, Thomas Gleixner 写道:
>> On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
>>
>>> +	tail = (tail + 1) % INVALID_QUEUE_SIZE;
>> Why is this before the barrier? The barrier does not do anything about
>> this and you can simplify this. See below.
>>
>> And as there is no rmb() counterpart you want to explain that this is
>> serializing against the hardware.
>>> +	 */
>>> +	wmb();
>>> +
>>> +	write_queue_tail(rqueue->node, tail);
>> 	write_queue_tail(rqueue->node, (tail + 1) & INVALID_QUEUE_MASK);
>>
>> No?
>
> The reason fo coding here is that during testing, it was found that a
> barrier is needed between the update of temporary variable 'tail' and
> the operation of register with 'write_queue_tail' , otherwise
> write_queue_tail will probabilistically fail to obtain the correct
> value.

How so?

tail is the software managed part of the ringbuffer which is shared with
the hardware, right?

So even if the compiler would be allowed to reevalutate tail after the
barrier (it is NOT), then tail would still contain the same value as
before, no?

The wmb() is required to ensure that the hardware can observe the full
write of the command _before_ it can observe the update to the tail
index.

Anything else is voodoo.

Thanks,

        tglx

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Tianyang Zhang]

Hi, Thomas

在 2025/6/23 下午4:27, Thomas Gleixner 写道:
> On Mon, Jun 23 2025 at 10:45, Tianyang Zhang wrote:
>> 在 2025/6/13 下午11:20, Thomas Gleixner 写道:
>>> On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
>>>
>>>> +	tail = (tail + 1) % INVALID_QUEUE_SIZE;
>>> Why is this before the barrier? The barrier does not do anything about
>>> this and you can simplify this. See below.
>>>
>>> And as there is no rmb() counterpart you want to explain that this is
>>> serializing against the hardware.
>>>> +	 */
>>>> +	wmb();
>>>> +
>>>> +	write_queue_tail(rqueue->node, tail);
>>> 	write_queue_tail(rqueue->node, (tail + 1) & INVALID_QUEUE_MASK);
>>>
>>> No?
>> The reason fo coding here is that during testing, it was found that a
>> barrier is needed between the update of temporary variable 'tail' and
>> the operation of register with 'write_queue_tail' , otherwise
>> write_queue_tail will probabilistically fail to obtain the correct
>> value.
> How so?
>
> tail is the software managed part of the ringbuffer which is shared with
> the hardware, right?
>
> So even if the compiler would be allowed to reevalutate tail after the
> barrier (it is NOT), then tail would still contain the same value as
> before, no?
>
> The wmb() is required to ensure that the hardware can observe the full
> write of the command _before_ it can observe the update to the tail
> index.
>
> Anything else is voodoo.
>
> Thanks,
>
>          tglx

In my previous understanding, tail'value is actually a part of 'full 
write of the command '.

We must ensure that tail is updated to the correct value first, and then 
write this value

into the register (perhaps by adding wmb in write_queue_tail ).

In other words, this is originally to prevent the write register 
instruction from being executed

out of order before updating tail.

The above is just my personal understanding

Thanks

Tianyang

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Thomas Gleixner]

On Mon, Jun 23 2025 at 17:33, Tianyang Zhang wrote:
> 在 2025/6/23 下午4:27, Thomas Gleixner 写道:
>> tail is the software managed part of the ringbuffer which is shared with
>> the hardware, right?
>>
>> So even if the compiler would be allowed to reevalutate tail after the
>> barrier (it is NOT), then tail would still contain the same value as
>> before, no?
>>
>> The wmb() is required to ensure that the hardware can observe the full
>> write of the command _before_ it can observe the update to the tail
>> index.
>>
>> Anything else is voodoo.
>>
>> Thanks,
>>
>>          tglx
>
> In my previous understanding, tail'value is actually a part of 'full 
> write of the command '.

Of course. The hardware observes the tail value. If it is not updated
then the command is not executed. But these are two distinct things:

The invalidate command is written to a location in the command buffer
which is determined by tail:

	inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
	memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));

requeue::base points to an array of invalidate commands. The array
size is INVALID_QUEUE_SIZE. tail is the position in the array to which
the software writes the next command. tail is software managed and
written to a completely different location via write_queue_tail(...):

static phys_addr_t redirect_reg_base = 0x1fe00000;

#define REDIRECT_REG_BASE(reg, node) \
	(UNCACHE_BASE | redirect_reg_base | (u64)(node) << NODE_ADDRSPACE_SHIFT | (reg))
#define	redirect_reg_queue_tail(node)	REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQT, (node))
#define read_queue_tail(node)		(*((u32 *)(redirect_reg_queue_tail(node))))
#define write_queue_tail(node, val)	(*((u32 *)(redirect_reg_queue_tail(node))) = (val))

The hardware maintains the head index. It's the last command index the
hardware processed. When the hardware observes that head != tail then it
processes the next entry and after completion it updates head with the
next index. This repeats until head == tail.

> We must ensure that tail is updated to the correct value first, and
> then write this value into the register (perhaps by adding wmb in
> write_queue_tail ).

No. The local variable 'tail' is purely local to the CPU and the
invalidation hardware does not even know that it exists.

There are two things which are relevant to the hardware:

   1) command is written to the hardware visible array at index of tail

   2) hardware visible tail memory (register) is updated to tail + 1

The memory barrier is required to prevent that #2 is written to the
hardware _before_ #1 completed and is fully visible to the hardware.

> In other words, this is originally to prevent the write register 
> instruction from being executed out of order before updating tail.

No. The barrier is solely for the above #1 vs. #2 ordering.

There is a difference between program flow ordering and memory ordering.

The hardware _CANNOT_ execute the write _before_ the value it writes is
computed. That's enforced by program flow order.

So it's always guaranteed that the CPU executes

   tail + 1

_before_ executing the write to the register because that's a program
order dependency.

If that would not be guaranteed, then your CPU would have more serious
problems than this piece of code. It simply would not even survive the
first five instructions in the boot loader.

But what's not guaranteed is that consecutive writes become visible to
other parts of the system (other CPUs, the invalidation hardware, ....)
in the same consecutive order.

To ensure that ordering you need a WMB(). If you would have CPU to CPU
communication then you would need a RMB() on the reader side to ensure
that the command is not read before the tail. In your case the hardware
side takes care of that.

> The above is just my personal understanding

Which might need some slight adjustments :)

Thanks,

        tglx

Re: [PATCH v4 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support

[Tianyang Zhang]

Hi, Thomas

在 2025/6/24 上午4:05, Thomas Gleixner 写道:
> On Mon, Jun 23 2025 at 17:33, Tianyang Zhang wrote:
>> 在 2025/6/23 下午4:27, Thomas Gleixner 写道:
>>> tail is the software managed part of the ringbuffer which is shared with
>>> the hardware, right?
>>>
>>> So even if the compiler would be allowed to reevalutate tail after the
>>> barrier (it is NOT), then tail would still contain the same value as
>>> before, no?
>>>
>>> The wmb() is required to ensure that the hardware can observe the full
>>> write of the command _before_ it can observe the update to the tail
>>> index.
>>>
>>> Anything else is voodoo.
>>>
>>> Thanks,
>>>
>>>           tglx
>> In my previous understanding, tail'value is actually a part of 'full
>> write of the command '.
> Of course. The hardware observes the tail value. If it is not updated
> then the command is not executed. But these are two distinct things:
>
> The invalidate command is written to a location in the command buffer
> which is determined by tail:
>
> 	inv_addr = (struct irde_inv_cmd *)(rqueue->base + tail * sizeof(struct irde_inv_cmd));
> 	memcpy(inv_addr, cmd, sizeof(struct irde_inv_cmd));
>
> requeue::base points to an array of invalidate commands. The array
> size is INVALID_QUEUE_SIZE. tail is the position in the array to which
> the software writes the next command. tail is software managed and
> written to a completely different location via write_queue_tail(...):
>
> static phys_addr_t redirect_reg_base = 0x1fe00000;
>
> #define REDIRECT_REG_BASE(reg, node) \
> 	(UNCACHE_BASE | redirect_reg_base | (u64)(node) << NODE_ADDRSPACE_SHIFT | (reg))
> #define	redirect_reg_queue_tail(node)	REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQT, (node))
> #define read_queue_tail(node)		(*((u32 *)(redirect_reg_queue_tail(node))))
> #define write_queue_tail(node, val)	(*((u32 *)(redirect_reg_queue_tail(node))) = (val))
>
> The hardware maintains the head index. It's the last command index the
> hardware processed. When the hardware observes that head != tail then it
> processes the next entry and after completion it updates head with the
> next index. This repeats until head == tail.
>
>> We must ensure that tail is updated to the correct value first, and
>> then write this value into the register (perhaps by adding wmb in
>> write_queue_tail ).
> No. The local variable 'tail' is purely local to the CPU and the
> invalidation hardware does not even know that it exists.
>
> There are two things which are relevant to the hardware:
>
>     1) command is written to the hardware visible array at index of tail
>
>     2) hardware visible tail memory (register) is updated to tail + 1
>
> The memory barrier is required to prevent that #2 is written to the
> hardware _before_ #1 completed and is fully visible to the hardware.
>
>> In other words, this is originally to prevent the write register
>> instruction from being executed out of order before updating tail.
> No. The barrier is solely for the above #1 vs. #2 ordering.
>
> There is a difference between program flow ordering and memory ordering.
>
> The hardware _CANNOT_ execute the write _before_ the value it writes is
> computed. That's enforced by program flow order.
>
> So it's always guaranteed that the CPU executes
>
>     tail + 1
>
> _before_ executing the write to the register because that's a program
> order dependency.
>
> If that would not be guaranteed, then your CPU would have more serious
> problems than this piece of code. It simply would not even survive the
> first five instructions in the boot loader.
>
> But what's not guaranteed is that consecutive writes become visible to
> other parts of the system (other CPUs, the invalidation hardware, ....)
> in the same consecutive order.
>
> To ensure that ordering you need a WMB(). If you would have CPU to CPU
> communication then you would need a RMB() on the reader side to ensure
> that the command is not read before the tail. In your case the hardware
> side takes care of that.
>
>> The above is just my personal understanding
> Which might need some slight adjustments :)
>
> Thanks,
>
>          tglx

Hmm... it seems I confused program-flow-ordering and memory-ordering .

my understanding of the previous issue may not have been right.

Your reply has taught me a great deal, truly appreciate it

Thank you very much

Tianyang

Re: [PATCH v4 0/2] Loongarch irq-redirect supprot

[Huacai Chen]

Hi, Tianyang,

Have you received my comments in V3?
https://lore.kernel.org/loongarch/20250523101833.17940-1-zhangtianyang@loongson.cn/T/#m2883f379ce7eb663f3f3eb4736bf9b071c7fd8ab

Huacai

On Tue, Jun 10, 2025 at 7:43 PM Tianyang Zhang
<zhangtianyang@loongson.cn> wrote:
>
> From: Super User <root@localhost.localdomain>
>
> This series of patches introduces support for interrupt-redirect
> controllers, and this hardware feature will be supported on 3C6000
> for the first time
>
> change log:
>         v0->v1:
>         1.Rename the model names in the document.
>         2.Adjust the code format.
>         3.Remove architecture - specific prefixes.
>         4.Refactor the initialization logic, and IR driver no longer set AVEC_ENABLE.
>         5.Enhance compatibility under certain configurations.
>
>         v1->v2:
>         1.Fixed an erroneous enabling issue.
>
>         v2->v3
>         1.Replace smp_call with address mapping to access registers
>         2.Fix some code style issues
>
>         v3->v4
>         1.Provide reasonable comments on the modifications made to IRQ_SET_MASK_OK_DONE
>         2.Replace meaningless empty functions with parent_mask/unmask/ack
>         3.Added and indeed released resources
>         4.Added judgment for data structure initialization completion to avoid duplicate creation during cpuhotplug
>         5.Fixed the code style and some unnecessary troubles
>
> Tianyang Zhang (2):
>   Docs/LoongArch: Add Advanced Extended-Redirect IRQ model description
>   irq/irq-loongarch-ir:Add Redirect irqchip support
>
>  .../arch/loongarch/irq-chip-model.rst         |  38 ++
>  .../zh_CN/arch/loongarch/irq-chip-model.rst   |  37 ++
>  arch/loongarch/include/asm/cpu-features.h     |   1 +
>  arch/loongarch/include/asm/cpu.h              |   2 +
>  arch/loongarch/include/asm/loongarch.h        |   6 +
>  arch/loongarch/kernel/cpu-probe.c             |   2 +
>  drivers/irqchip/Makefile                      |   2 +-
>  drivers/irqchip/irq-loongarch-avec.c          |  25 +-
>  drivers/irqchip/irq-loongarch-ir.c            | 562 ++++++++++++++++++
>  drivers/irqchip/irq-loongson.h                |  12 +
>  include/linux/cpuhotplug.h                    |   1 +
>  11 files changed, 674 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/irqchip/irq-loongarch-ir.c
>
> --
> 2.41.0
>

Re: [PATCH v4 0/2] Loongarch irq-redirect supprot

[Tianyang Zhang]

Hi, Huacai

在 2025/6/10 下午7:54, Huacai Chen 写道:
> Hi, Tianyang,
>
> Have you received my comments in V3?
> https://lore.kernel.org/loongarch/20250523101833.17940-1-zhangtianyang@loongson.cn/T/#m2883f379ce7eb663f3f3eb4736bf9b071c7fd8ab
>
> Huacai

Sorry, except for a slight network issue, I did not receive the email 
mentioned above.

I will reply to the email first and continue to modify this series of 
patches

Tianyang

> On Tue, Jun 10, 2025 at 7:43 PM Tianyang Zhang
> <zhangtianyang@loongson.cn> wrote:
>> From: Super User <root@localhost.localdomain>
>>
>> This series of patches introduces support for interrupt-redirect
>> controllers, and this hardware feature will be supported on 3C6000
>> for the first time
>>
>> change log:
>>          v0->v1:
>>          1.Rename the model names in the document.
>>          2.Adjust the code format.
>>          3.Remove architecture - specific prefixes.
>>          4.Refactor the initialization logic, and IR driver no longer set AVEC_ENABLE.
>>          5.Enhance compatibility under certain configurations.
>>
>>          v1->v2:
>>          1.Fixed an erroneous enabling issue.
>>
>>          v2->v3
>>          1.Replace smp_call with address mapping to access registers
>>          2.Fix some code style issues
>>
>>          v3->v4
>>          1.Provide reasonable comments on the modifications made to IRQ_SET_MASK_OK_DONE
>>          2.Replace meaningless empty functions with parent_mask/unmask/ack
>>          3.Added and indeed released resources
>>          4.Added judgment for data structure initialization completion to avoid duplicate creation during cpuhotplug
>>          5.Fixed the code style and some unnecessary troubles
>>
>> Tianyang Zhang (2):
>>    Docs/LoongArch: Add Advanced Extended-Redirect IRQ model description
>>    irq/irq-loongarch-ir:Add Redirect irqchip support
>>
>>   .../arch/loongarch/irq-chip-model.rst         |  38 ++
>>   .../zh_CN/arch/loongarch/irq-chip-model.rst   |  37 ++
>>   arch/loongarch/include/asm/cpu-features.h     |   1 +
>>   arch/loongarch/include/asm/cpu.h              |   2 +
>>   arch/loongarch/include/asm/loongarch.h        |   6 +
>>   arch/loongarch/kernel/cpu-probe.c             |   2 +
>>   drivers/irqchip/Makefile                      |   2 +-
>>   drivers/irqchip/irq-loongarch-avec.c          |  25 +-
>>   drivers/irqchip/irq-loongarch-ir.c            | 562 ++++++++++++++++++
>>   drivers/irqchip/irq-loongson.h                |  12 +
>>   include/linux/cpuhotplug.h                    |   1 +
>>   11 files changed, 674 insertions(+), 14 deletions(-)
>>   create mode 100644 drivers/irqchip/irq-loongarch-ir.c
>>
>> --
>> 2.41.0
>>

Re: [PATCH v4 0/2] Loongarch irq-redirect supprot

[Tianyang Zhang]

Hi, Huacai

在 2025/6/10 下午7:54, Huacai Chen 写道:
> Hi, Tianyang,
>
> Have you received my comments in V3?
> https://lore.kernel.org/loongarch/20250523101833.17940-1-zhangtianyang@loongson.cn/T/#m2883f379ce7eb663f3f3eb4736bf9b071c7fd8ab

After a few days of effort, I did not let that email reappear in my 
mailbox... so I am replying to the above email here

* Re: [PATCH v3 2/2] irq/irq-loongarch-ir:Add Redirect irqchip support
   2025-05-23 10:18 ` [PATCH v3 2/2] irq/irq-loongarch-ir:Add Redirect 
irqchip support Tianyang Zhang
@ 2025-05-24 14:12   ` Huacai Chen
   2025-05-25  9:06   ` Thomas Gleixner
   1 sibling, 0 replies; 7+ messages in thread
From: Huacai Chen @ 2025-05-24 14:12 UTC (permalink / raw)
   To: Tianyang Zhang
   Cc: kernel, corbet, alexs, si.yanteng, tglx, jiaxun.yang, peterz,
     wangliupu, lvjianmin, maobibo, siyanteng, gaosong, yangtiezhu,
     loongarch, linux-doc, linux-kernel

 >> +
 >> +#define REDIRECT_REG_BASE(reg, node) \
 >> +       (UNCACHE_BASE | redirect_reg_base | (u64)(node) << 
NODE_ADDRSPACE_SHIFT | (reg))
 > IO_BASE is a little better than UNCACHE_BASE.

yes, it is batter, thanks

 >> +#define        redirect_reg_queue_head(node) 
REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQH, (node))
 >> +#define        redirect_reg_queue_tail(node) 
REDIRECT_REG_BASE(LOONGARCH_IOCSR_REDIRECT_CQT, (node))
 >> +#define read_queue_head(node)          (*((u32 
*)(redirect_reg_queue_head(node))))
 >> +#define read_queue_tail(node)          (*((u32 
*)(redirect_reg_queue_tail(node))))
 >> +#define write_queue_tail(node, val)    (*((u32 
*)(redirect_reg_queue_tail(node))) = (val))
 >You can use readl() and writel() directly, then you can remove the 
memory barrier around write_queue_tail().

OK , It is realy a good idea.thanks

 >> +static void irde_invlid_entry_node(struct redirect_item *item)
 > s/irde_invlid_entry_node/irde_invalid_entry_node/g

OK , thanks

 >> +       avecintc_sync(adata);
 >> +
 >> +       return IRQ_SET_MASK_OK;
 >> +}
 > Have you tried to build with no SMP? This function (and maybe more) 
should be guarded by CONFIG_SMP.

I did it at the beginning... Okay, I will supplement this test in the 
current version

 >> +static int __init redirect_reg_base_init(void)
 >> +{
 >> +       acpi_status status;
 >> +       uint64_t addr = 0;
 >> +
 >> +       if (acpi_disabled)
 >> +               return 0;
 >> +
 >> +       status = acpi_evaluate_integer(NULL, "\\_SB.NO00", NULL, &addr);
 >> +       if (ACPI_FAILURE(status) || !addr)
 >> +               pr_info("redirect_iocsr_base used default 
0x1fe00000\n");
 >> +       else
 >> +               redirect_reg_base = addr;
 >> +
 >> +       return 0;
 >> +}
 >> +subsys_initcall_sync(redirect_reg_base_init);
 >Can this function be put at the end of redirect_acpi_init()? It is too
 >late in an initcall() function because the irqchip drivers begin to
 >work before that.
Ok I got it , thanks

Tianyang

Re: [PATCH v4 0/2] Loongarch irq-redirect supprot

[Thomas Gleixner]

On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
> From: Super User <root@localhost.localdomain>

That's a valid developer name :)

> This series of patches introduces support for interrupt-redirect
> controllers, and this hardware feature will be supported on 3C6000
> for the first time
>
> change log:
> 	v3->v4
> 	1.Provide reasonable comments on the modifications made to IRQ_SET_MASK_OK_DONE	

That's not really what I asked for:

  "This change really wants to be seperate with a proper explanation and
   not burried inside of this pile of changes."

Emphasis on _seperate_, which translates to:

  "Put it into a seperate patch with a proper changelog explaining this
   modification and why it is correct."

You still have burried this in the whole pile of unrelated changes.

Thanks,

        tglx

Re: [PATCH v4 0/2] Loongarch irq-redirect supprot

[Tianyang Zhang]

Hi, Thomas

在 2025/6/13 下午10:02, Thomas Gleixner 写道:
> On Tue, Jun 10 2025 at 19:42, Tianyang Zhang wrote:
>> From: Super User <root@localhost.localdomain>
> That's a valid developer name :)
Sorry , it's a realy stupid fault.....
>
>> This series of patches introduces support for interrupt-redirect
>> controllers, and this hardware feature will be supported on 3C6000
>> for the first time
>>
>> change log:
>> 	v3->v4
>> 	1.Provide reasonable comments on the modifications made to IRQ_SET_MASK_OK_DONE	
> That's not really what I asked for:
>
>    "This change really wants to be seperate with a proper explanation and
>     not burried inside of this pile of changes."
>
> Emphasis on _seperate_, which translates to:
>
>    "Put it into a seperate patch with a proper changelog explaining this
>     modification and why it is correct."
>
> You still have burried this in the whole pile of unrelated changes.
>
> Thanks,
>
>          tglx

Okay, I thought it was just a dissatisfaction with the "inclusion" of 
some changes. I will try to modify it here


Tianyang