Re: [PATCH] irqchip/loongson-eiointc: Add extioi virt extension support
From: gaosong
Date: Thu Jun 06 2024 - 08:59:24 EST
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/20240528083855.1912757-1-gaosong@xxxxxxxxxxx/
+#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