2024-06-05 07:25:34

by Song Gao

[permalink] [raw]
Subject: [PATCH] irqchip/loongson-eiointc: Add extioi virt extension support

Currently IRQs can be routed to only 4 vcpus with one Hw extioi.
This patch adds the extioi virt extension support so that IRQs can
be routed to 256 vcpus on hypervior mode.

Now IRQs is emulated in userspace, so the extioi virt device emulation
is implemented in userspace firstly.

See:
https://patchew.org/QEMU/[email protected]/

Signed-off-by: Song Gao <[email protected]>
---
arch/loongarch/include/asm/irq.h | 1 +
drivers/irqchip/irq-loongson-eiointc.c | 75 +++++++++++++++++++++-----
2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
index 480418bc5071..95542515b66e 100644
--- a/arch/loongarch/include/asm/irq.h
+++ b/arch/loongarch/include/asm/irq.h
@@ -53,6 +53,7 @@ struct acpi_vector_group {
extern struct acpi_vector_group pch_group[MAX_IO_PICS];
extern struct acpi_vector_group msi_group[MAX_IO_PICS];

+#define MAX_CORES_PER_EIO_NODE 256
#define CORES_PER_EIO_NODE 4

#define LOONGSON_CPU_UART0_VEC 10 /* CPU UART0 */
diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index c7ddebf312ad..073445d64e3b 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -23,6 +23,16 @@
#define EIOINTC_REG_ISR 0x1800
#define EIOINTC_REG_ROUTE 0x1c00

+#define EXTIOI_VIRT_FEATURES 0x40000000
+#define EXTIOI_HAS_VIRT_EXTENSION 0
+#define EXTIOI_HAS_ENABLE_OPTION 1
+#define EXTIOI_HAS_INT_ENCODE 2
+#define EXTIOI_HAS_CPU_ENCODE 3
+#define EXTIOI_VIRT_CONFIG 0x40000004
+#define EXTIOI_ENABLE 1
+#define EXTIOI_ENABLE_INT_ENCODE 2
+#define EXTIOI_ENABLE_CPU_ENCODE 3
+
#define VEC_REG_COUNT 4
#define VEC_COUNT_PER_REG 64
#define VEC_COUNT (VEC_REG_COUNT * VEC_COUNT_PER_REG)
@@ -41,6 +51,7 @@ struct eiointc_priv {
cpumask_t cpuspan_map;
struct fwnode_handle *domain_handle;
struct irq_domain *eiointc_domain;
+ bool cpu_encoded;
};

static struct eiointc_priv *eiointc_priv[MAX_IO_PICS];
@@ -56,7 +67,9 @@ static void eiointc_enable(void)

static int cpu_to_eio_node(int cpu)
{
- return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
+ int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);
+
+ return cpu_logical_map(cpu) / cores;
}

#ifdef CONFIG_SMP
@@ -88,6 +101,20 @@ static void eiointc_set_irq_route(int pos, unsigned int cpu, unsigned int mnode,

static DEFINE_RAW_SPINLOCK(affinity_lock);

+static void virt_extioi_set_irq_route(int irq, unsigned int cpu)
+{
+ int data;
+
+ /*
+ * get irq route info for continuous 4 vectors
+ * and set affinity for specified vector
+ */
+ data = iocsr_read32(EIOINTC_REG_ROUTE + (irq & ~3));
+ data &= ~(0xff << ((irq & 3) * 8));
+ data |= cpu_logical_map(cpu) << ((irq & 3) * 8);
+ iocsr_write32(data, EIOINTC_REG_ROUTE + (irq & ~3));
+}
+
static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *affinity, bool force)
{
unsigned int cpu;
@@ -106,16 +133,22 @@ static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *af
vector = d->hwirq;
regaddr = EIOINTC_REG_ENABLE + ((vector >> 5) << 2);

- /* Mask target vector */
- csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
- 0x0, priv->node * CORES_PER_EIO_NODE);
-
- /* Set route for target vector */
- eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
-
- /* Unmask target vector */
- csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
- 0x0, priv->node * CORES_PER_EIO_NODE);
+ if (priv->cpu_encoded) {
+ iocsr_write32(EIOINTC_ALL_ENABLE & ~BIT(vector & 0x1F), regaddr);
+ virt_extioi_set_irq_route(vector, cpu);
+ iocsr_write32(EIOINTC_ALL_ENABLE, regaddr);
+ } else {
+ /* Mask target vector */
+ csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
+ 0x0, priv->node * CORES_PER_EIO_NODE);
+
+ /* Set route for target vector */
+ eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
+
+ /* Unmask target vector */
+ csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
+ 0x0, priv->node * CORES_PER_EIO_NODE);
+ }

irq_data_update_effective_affinity(d, cpumask_of(cpu));

@@ -143,13 +176,14 @@ static int eiointc_router_init(unsigned int cpu)
uint32_t data;
uint32_t node = cpu_to_eio_node(cpu);
int index = eiointc_index(node);
+ int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);

if (index < 0) {
pr_err("Error: invalid nodemap!\n");
return -1;
}

- if ((cpu_logical_map(cpu) % CORES_PER_EIO_NODE) == 0) {
+ if ((cpu_logical_map(cpu) % cores) == 0) {
eiointc_enable();

for (i = 0; i < eiointc_priv[0]->vec_count / 32; i++) {
@@ -165,7 +199,9 @@ static int eiointc_router_init(unsigned int cpu)

for (i = 0; i < eiointc_priv[0]->vec_count / 4; i++) {
/* Route to Node-0 Core-0 */
- if (index == 0)
+ if (eiointc_priv[index]->cpu_encoded)
+ bit = cpu_logical_map(0);
+ else if (index == 0)
bit = BIT(cpu_logical_map(0));
else
bit = (eiointc_priv[index]->node << 4) | 1;
@@ -369,7 +405,7 @@ static int __init acpi_cascade_irqdomain_init(void)
static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
u64 node_map)
{
- int i;
+ int i, val;

node_map = node_map ? node_map : -1ULL;
for_each_possible_cpu(i) {
@@ -389,6 +425,17 @@ static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
return -ENOMEM;
}

+ if (cpu_has_hypervisor) {
+ val = iocsr_read32(EXTIOI_VIRT_FEATURES);
+ if (val & BIT(EXTIOI_HAS_CPU_ENCODE)) {
+ val = iocsr_read32(EXTIOI_VIRT_CONFIG);
+ val |= BIT(EXTIOI_ENABLE_CPU_ENCODE);
+ iocsr_write32(val, EXTIOI_VIRT_CONFIG);
+ priv->cpu_encoded = true;
+ pr_info("loongson-extioi: enable cpu encodig \n");
+ }
+ }
+
eiointc_priv[nr_pics++] = priv;
eiointc_router_init(0);
irq_set_chained_handler_and_data(parent_irq, eiointc_irq_dispatch, priv);
--
2.39.3



2024-06-05 17:36:44

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] irqchip/loongson-eiointc: Add extioi virt extension support

On Wed, Jun 05 2024 at 15:02, Song Gao wrote:
> Currently IRQs can be routed to only 4 vcpus with one Hw extioi.

Can you please use proper words instead of acronyms?

Interrupts can be routed to maximal four virtual CPUs with one external
hardware interrupt.

> This patch adds the extioi virt extension support so that IRQs can

git grep 'This patch' Documentation/process/

> be routed to 256 vcpus on hypervior mode.
>
> Now IRQs is emulated in userspace, so the extioi virt device emulation
> is implemented in userspace firstly.

I'm failing to understand what this means and how this and the link are
relevant to the change log and the change itself.

> See:
> https://patchew.org/QEMU/[email protected]/
>
> +#define EXTIOI_VIRT_FEATURES 0x40000000
> +#define EXTIOI_HAS_VIRT_EXTENSION 0
> +#define EXTIOI_HAS_ENABLE_OPTION 1
> +#define EXTIOI_HAS_INT_ENCODE 2
> +#define EXTIOI_HAS_CPU_ENCODE 3
> +#define EXTIOI_VIRT_CONFIG 0x40000004
> +#define EXTIOI_ENABLE 1
> +#define EXTIOI_ENABLE_INT_ENCODE 2
> +#define EXTIOI_ENABLE_CPU_ENCODE 3

These are clearly bits. So why not define them as BIT(0), so you can
spare that at the usage site?

Also why defining all of this when there are only two defines used?

> +
> #define VEC_REG_COUNT 4
> #define VEC_COUNT_PER_REG 64
> #define VEC_COUNT (VEC_REG_COUNT * VEC_COUNT_PER_REG)
> @@ -41,6 +51,7 @@ struct eiointc_priv {
> cpumask_t cpuspan_map;
> struct fwnode_handle *domain_handle;
> struct irq_domain *eiointc_domain;
> + bool cpu_encoded;
> };
>
> static struct eiointc_priv *eiointc_priv[MAX_IO_PICS];
> @@ -56,7 +67,9 @@ static void eiointc_enable(void)
>
> static int cpu_to_eio_node(int cpu)
> {
> - return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
> + int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);

Pointless brackets.

> +
> + return cpu_logical_map(cpu) / cores;
> }
>
> #ifdef CONFIG_SMP
> @@ -88,6 +101,20 @@ static void eiointc_set_irq_route(int pos, unsigned int cpu, unsigned int mnode,
>
> static DEFINE_RAW_SPINLOCK(affinity_lock);
>
> +static void virt_extioi_set_irq_route(int irq, unsigned int cpu)

unsigned int irq;

But this is about vectors not interrupts. So please name it
accordingly. 'irq' is just confusing. I had to look up the call site to
figure out what this is about.

> +{
> + int data;

This represents a hardware register value, so the data type wants to be
u32, no?

> + /*
> + * get irq route info for continuous 4 vectors
> + * and set affinity for specified vector
> + */
> + data = iocsr_read32(EIOINTC_REG_ROUTE + (irq & ~3));
> + data &= ~(0xff << ((irq & 3) * 8));
> + data |= cpu_logical_map(cpu) << ((irq & 3) * 8);
> + iocsr_write32(data, EIOINTC_REG_ROUTE + (irq & ~3));

This all consists of undocumented magic numeric constants '3', '0xff',
8. Documentation clearly tells not to do so.

/*
* Routing registers contain four vectors and have an offset of four to
* the base. The routing information is 8 bit wide.
*/
#define EIOINTC_REG_ROUTE(vector) \
(0x1c00 + (vector & ~0x03)

#define EIOINTC_REG_ROUTE_SHIFT(vector) \
((vector & 0x03) << 3)

#define EIOINTC_REG_ROUTE_MASK(vector) \
(0xff << EIOINTC_REG_ROUTE_SHIFT(vector))

Those can be used to simplify the existing code pretty much in a
preparatory patch and then the above becomes:

unsigned long reg = EIOINTC_REG_ROUTE(vector);
u32 data = iocsr_read32(reg);

data &= EIOINTC_REG_ROUTE_MASK(vector);
data |= cpu_logical_map(cpu) << EIOINTC_REG_ROUTE_SHIFT(vector);
iocsr_write32(reg, data);

See how this makes the code suddenly comprehensible?

> static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *affinity, bool force)
> {
> unsigned int cpu;
> @@ -106,16 +133,22 @@ static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *af
> vector = d->hwirq;
> regaddr = EIOINTC_REG_ENABLE + ((vector >> 5) << 2);
>
> - /* Mask target vector */
> - csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
> - 0x0, priv->node * CORES_PER_EIO_NODE);
> -
> - /* Set route for target vector */
> - eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
> -
> - /* Unmask target vector */
> - csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
> - 0x0, priv->node * CORES_PER_EIO_NODE);
> + if (priv->cpu_encoded) {
> + iocsr_write32(EIOINTC_ALL_ENABLE & ~BIT(vector & 0x1F), regaddr);

There exists BIT_MASK() and please get rid of these numeric constants.

> + virt_extioi_set_irq_route(vector, cpu);
> + iocsr_write32(EIOINTC_ALL_ENABLE, regaddr);
> + } else {
> + /* Mask target vector */
> + csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
> + 0x0, priv->node * CORES_PER_EIO_NODE);
> +
> + /* Set route for target vector */
> + eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
> +
> + /* Unmask target vector */
> + csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
> + 0x0, priv->node * CORES_PER_EIO_NODE);

Please follow the documented tip tree coding style for line breaks.

> + }
>
> irq_data_update_effective_affinity(d, cpumask_of(cpu));
>
> @@ -143,13 +176,14 @@ static int eiointc_router_init(unsigned int cpu)
> uint32_t data;
> uint32_t node = cpu_to_eio_node(cpu);
> int index = eiointc_index(node);
> + int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);

Sigh. Please use the documented ordering of variable declaration
ordering.

> if (index < 0) {
> pr_err("Error: invalid nodemap!\n");
> return -1;

What is -1? Error codes exist for a reason.

> static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
> u64 node_map)
> {
> - int i;
> + int i, val;

again u32 ...

> node_map = node_map ? node_map : -1ULL;
> for_each_possible_cpu(i) {
> @@ -389,6 +425,17 @@ static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
> return -ENOMEM;
> }
>
> + if (cpu_has_hypervisor) {
> + val = iocsr_read32(EXTIOI_VIRT_FEATURES);
> + if (val & BIT(EXTIOI_HAS_CPU_ENCODE)) {
> + val = iocsr_read32(EXTIOI_VIRT_CONFIG);
> + val |= BIT(EXTIOI_ENABLE_CPU_ENCODE);
> + iocsr_write32(val, EXTIOI_VIRT_CONFIG);
> + priv->cpu_encoded = true;
> + pr_info("loongson-extioi: enable cpu encodig \n");
> + }

Thanks,

tglx

2024-06-06 13:01:13

by Song Gao

[permalink] [raw]
Subject: Re: [PATCH] irqchip/loongson-eiointc: Add extioi virt extension support

Hi Thomas,

Thanks for your comments

在 2024/6/6 上午1:36, Thomas Gleixner 写道:
> On Wed, Jun 05 2024 at 15:02, Song Gao wrote:
>> Currently IRQs can be routed to only 4 vcpus with one Hw extioi.
> Can you please use proper words instead of acronyms?
>
> Interrupts can be routed to maximal four virtual CPUs with one external
> hardware interrupt.
Yes,  I will correct it.
>> This patch adds the extioi virt extension support so that IRQs can
> git grep 'This patch' Documentation/process/
>
>> be routed to 256 vcpus on hypervior mode.
>>
>> Now IRQs is emulated in userspace, so the extioi virt device emulation
>> is implemented in userspace firstly.
> I'm failing to understand what this means and how this and the link are
> relevant to the change log and the change itself.
I will clean it.
>> See:
>> https://patchew.org/QEMU/[email protected]/
>>
>> +#define EXTIOI_VIRT_FEATURES 0x40000000
>> +#define EXTIOI_HAS_VIRT_EXTENSION 0
>> +#define EXTIOI_HAS_ENABLE_OPTION 1
>> +#define EXTIOI_HAS_INT_ENCODE 2
>> +#define EXTIOI_HAS_CPU_ENCODE 3
>> +#define EXTIOI_VIRT_CONFIG 0x40000004
>> +#define EXTIOI_ENABLE 1
>> +#define EXTIOI_ENABLE_INT_ENCODE 2
>> +#define EXTIOI_ENABLE_CPU_ENCODE 3
> These are clearly bits. So why not define them as BIT(0), so you can
> spare that at the usage site?
Got it.
> Also why defining all of this when there are only two defines used?
BIT(1) and BIT(2) correspond to Bit Fields 48 and 49 of Table 28 in
Section 4.13 of Manual [1].
These are present in the physical hardware, just not yet used in the
software logic.

[1]
https://github.com/loongson/LoongArch-Documentation/releases/download/2023.04.20/Loongson-3A5000-usermanual-v1.03-EN.pdf

>> +
>> #define VEC_REG_COUNT 4
>> #define VEC_COUNT_PER_REG 64
>> #define VEC_COUNT (VEC_REG_COUNT * VEC_COUNT_PER_REG)
>> @@ -41,6 +51,7 @@ struct eiointc_priv {
>> cpumask_t cpuspan_map;
>> struct fwnode_handle *domain_handle;
>> struct irq_domain *eiointc_domain;
>> + bool cpu_encoded;
>> };
>>
>> static struct eiointc_priv *eiointc_priv[MAX_IO_PICS];
>> @@ -56,7 +67,9 @@ static void eiointc_enable(void)
>>
>> static int cpu_to_eio_node(int cpu)
>> {
>> - return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
>> + int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);
> Pointless brackets.
Got it.
>> +
>> + return cpu_logical_map(cpu) / cores;
>> }
>>
>> #ifdef CONFIG_SMP
>> @@ -88,6 +101,20 @@ static void eiointc_set_irq_route(int pos, unsigned int cpu, unsigned int mnode,
>>
>> static DEFINE_RAW_SPINLOCK(affinity_lock);
>>
>> +static void virt_extioi_set_irq_route(int irq, unsigned int cpu)
> unsigned int irq;
>
> But this is about vectors not interrupts. So please name it
> accordingly. 'irq' is just confusing. I had to look up the call site to
> figure out what this is about.
Thanks for your suggestion.
>> +{
>> + int data;
> This represents a hardware register value, so the data type wants to be
> u32, no?
I will correct it.
>> + /*
>> + * get irq route info for continuous 4 vectors
>> + * and set affinity for specified vector
>> + */
>> + data = iocsr_read32(EIOINTC_REG_ROUTE + (irq & ~3));
>> + data &= ~(0xff << ((irq & 3) * 8));
>> + data |= cpu_logical_map(cpu) << ((irq & 3) * 8);
>> + iocsr_write32(data, EIOINTC_REG_ROUTE + (irq & ~3));
> This all consists of undocumented magic numeric constants '3', '0xff',
> 8. Documentation clearly tells not to do so.
>
> /*
> * Routing registers contain four vectors and have an offset of four to
> * the base. The routing information is 8 bit wide.
> */
> #define EIOINTC_REG_ROUTE(vector) \
> (0x1c00 + (vector & ~0x03)
>
> #define EIOINTC_REG_ROUTE_SHIFT(vector) \
> ((vector & 0x03) << 3)
>
> #define EIOINTC_REG_ROUTE_MASK(vector) \
> (0xff << EIOINTC_REG_ROUTE_SHIFT(vector))
>
> Those can be used to simplify the existing code pretty much in a
> preparatory patch and then the above becomes:
>
> unsigned long reg = EIOINTC_REG_ROUTE(vector);
> u32 data = iocsr_read32(reg);
>
> data &= EIOINTC_REG_ROUTE_MASK(vector);
> data |= cpu_logical_map(cpu) << EIOINTC_REG_ROUTE_SHIFT(vector);
> iocsr_write32(reg, data);
>
> See how this makes the code suddenly comprehensible?
Yes, the code is very concise, thanks for the example.
>> static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *affinity, bool force)
>> {
>> unsigned int cpu;
>> @@ -106,16 +133,22 @@ static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *af
>> vector = d->hwirq;
>> regaddr = EIOINTC_REG_ENABLE + ((vector >> 5) << 2);
>>
>> - /* Mask target vector */
>> - csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
>> - 0x0, priv->node * CORES_PER_EIO_NODE);
>> -
>> - /* Set route for target vector */
>> - eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
>> -
>> - /* Unmask target vector */
>> - csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
>> - 0x0, priv->node * CORES_PER_EIO_NODE);
>> + if (priv->cpu_encoded) {
>> + iocsr_write32(EIOINTC_ALL_ENABLE & ~BIT(vector & 0x1F), regaddr);
> There exists BIT_MASK() and please get rid of these numeric constants.
How about this:

  #define EIOINTC_ALL_ENABLE     0xffffffff
+#define EIOINTC_ALL_ENABLE_MASK(vector)        \
+           (EIOINTC_ALL_ENABLE & ~BIT(vector & 0x1F))
+#define EIOINTC_REG_ENABLE(vector)                 \
+           (EIOINTC_REG_ENABLE + (vector >> 5) << 2)

>> + virt_extioi_set_irq_route(vector, cpu);
>> + iocsr_write32(EIOINTC_ALL_ENABLE, regaddr);
>> + } else {
>> + /* Mask target vector */
>> + csr_any_send(regaddr, EIOINTC_ALL_ENABLE & (~BIT(vector & 0x1F)),
>> + 0x0, priv->node * CORES_PER_EIO_NODE);
>> +
>> + /* Set route for target vector */
>> + eiointc_set_irq_route(vector, cpu, priv->node, &priv->node_map);
>> +
>> + /* Unmask target vector */
>> + csr_any_send(regaddr, EIOINTC_ALL_ENABLE,
>> + 0x0, priv->node * CORES_PER_EIO_NODE);
> Please follow the documented tip tree coding style for line breaks.
OK.
>> + }
>>
>> irq_data_update_effective_affinity(d, cpumask_of(cpu));
>>
>> @@ -143,13 +176,14 @@ static int eiointc_router_init(unsigned int cpu)
>> uint32_t data;
>> uint32_t node = cpu_to_eio_node(cpu);
>> int index = eiointc_index(node);
>> + int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);
> Sigh. Please use the documented ordering of variable declaration
> ordering.

Sorry, I didn't find the information in document [1].

[1]: https://www.kernel.org/doc/html/next/process/coding-style.html


How about this:

static int eiointc_router_init(unsigned int cpu)
{
- int i, bit;
- uint32_t data;
- uint32_t node = cpu_to_eio_node(cpu);
- int index = eiointc_index(node);
- int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);
+ int i, bit, cores, index;
+ uint32_t data, node;
+
+ node = cpu_to_eio_node(cpu);
+ index = eiointc_index(node);

if (index < 0) {
pr_err("Error: invalid nodemap!\n");
return -1;
}

+ cores = cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE;
+
if ((cpu_logical_map(cpu) % cores) == 0) {

>> if (index < 0) {
>> pr_err("Error: invalid nodemap!\n");
>> return -1;
> What is -1? Error codes exist for a reason.
I will use -EINVAL.
>> static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
>> u64 node_map)
>> {
>> - int i;
>> + int i, val;
> again u32 ...
Got it .

Thanks again for your comments.

Thanks
Song Gao
>
>> node_map = node_map ? node_map : -1ULL;
>> for_each_possible_cpu(i) {
>> @@ -389,6 +425,17 @@ static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
>> return -ENOMEM;
>> }
>>
>> + if (cpu_has_hypervisor) {
>> + val = iocsr_read32(EXTIOI_VIRT_FEATURES);
>> + if (val & BIT(EXTIOI_HAS_CPU_ENCODE)) {
>> + val = iocsr_read32(EXTIOI_VIRT_CONFIG);
>> + val |= BIT(EXTIOI_ENABLE_CPU_ENCODE);
>> + iocsr_write32(val, EXTIOI_VIRT_CONFIG);
>> + priv->cpu_encoded = true;
>> + pr_info("loongson-extioi: enable cpu encodig \n");
>> + }
> Thanks,
>
> tglx


2024-06-06 14:52:28

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] irqchip/loongson-eiointc: Add extioi virt extension support

Song!

On Thu, Jun 06 2024 at 20:58, gaosong wrote:
> 在 2024/6/6 上午1:36, Thomas Gleixner 写道:
>> On Wed, Jun 05 2024 at 15:02, Song Gao wrote:
>>> int index = eiointc_index(node);
>>> + int cores = (cpu_has_hypervisor ? MAX_CORES_PER_EIO_NODE : CORES_PER_EIO_NODE);
>> Sigh. Please use the documented ordering of variable declaration
>> ordering.
>
> Sorry, I didn't find the information in document [1].
>
> [1]: https://www.kernel.org/doc/html/next/process/coding-style.html

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes

Thanks,

tglx