Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
Hi Richard On 2022/5/19 上午2:04, Richard Henderson wrote: + uint64_t sw_isr[LOONGARCH_MAX_VCPUS][LS3A_INTC_IP][EXTIOI_IRQS / 64]; This has not been declared with DECLARE_BITMAP, therefore you will see a compile-time error when building on an ILP32 (i686) or P64 (win64) host. I pointed this out to you as recently as v2 of this series. I am really disappointed to see this regress in just one month. You can test this yourself with IMAGES='fedora-i386-cross fedora-win32-cross fedora-win64-cross' \ make docker-test-build Please do so before your next submission. Thank you for your patient guidance, we will carefully correct them. Thanks. Xiaojuan
Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 5/17/22 04:30, Xiaojuan Yang wrote:
+static void extioi_update_irq(LoongArchExtIOI *s, int irq_num, int level)
+{
+int ipnum, cpu, found, irq_index, irq_mask;
+
+ipnum = get_ipmap(s, irq_num);
+cpu = get_coremap(s, irq_num);
+irq_index = irq_num / 32;
+irq_mask = 1 << (irq_num & 0x1f);
+
+if (level) {
+/* if not enable return false */
+if (((s->enable[irq_index]) & irq_mask) == 0) {
+s->sw_pending[irq_index] |= irq_mask;
+return;
+}
+s->coreisr[cpu][irq_index] |= irq_mask;
+found = find_first_bit(s->sw_isr[cpu][ipnum], EXTIOI_IRQS);
AGAIN! You CANNOT use only part of the bitmap.h interface.
+uint64_t sw_isr[LOONGARCH_MAX_VCPUS][LS3A_INTC_IP][EXTIOI_IRQS / 64];
This has not been declared with DECLARE_BITMAP, therefore you will see a compile-time
error when building on an ILP32 (i686) or P64 (win64) host.
I pointed this out to you as recently as v2 of this series.
I am really disappointed to see this regress in just one month.
You can test this yourself with
IMAGES='fedora-i386-cross fedora-win32-cross fedora-win64-cross' \
make docker-test-build
Please do so before your next submission.
+static void extioi_writew(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
+int i, cpu, index, old_data, data_offset;
+int old_ip, new_ip, old_core, new_core, irq_mask, irq_num;
+uint32_t offset;
+int old_ipnum[128], old_cpu[4];
+trace_loongarch_extioi_writew(size, (uint32_t)addr, val);
+
+offset = addr & 0x;
+
+switch (offset) {
+case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
+index = (offset - EXTIOI_NODETYPE_START) >> 2;
+s->nodetype[index] = val;
+break;
+case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
+/* get irq number */
+offset -= EXTIOI_IPMAP_START;
+index = offset >> 2;
+/*
+ * 4 bytes writing, set 4 irq groups one time,
+ * and one group is 32 irqs, so set 128 irqs mapping
+ */
+for (i = 0; i < 128; i++) {
+old_ipnum[i] = get_ipmap(s, offset);
+offset += 1;
+}
You increment offset in the first loop,
+s->ipmap[index] = val;
+offset -= 128;
+/* if core isr is set, need to update irq */
+for (i = 0; i < 128; i++) {
+old_ip = old_ipnum[i];
+new_ip = get_ipmap(s, offset);
+cpu = get_coremap(s, offset);
+if (old_ip != new_ip) {
+if (s->coreisr[cpu][offset / 32] & (1 << (offset & 0x1f))) {
+extioi_update_irq(s, offset, 1);
+}
+}
+}
but not the second.
+case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
+index = (offset - EXTIOI_ENABLE_START) >> 2;
+old_data = s->enable[index];
+if (old_data != (int)val) {
+s->enable[index] = val;
+/* get data diff */
+old_data ^= val;
+/* value change from 0 to 1 */
+old_data &= val;
+data_offset = ctz32(old_data);
+while (data_offset != 32) {
+/*
+ * enable bit change from 0 to 1,
+ * need to update irq by pending bits
+ */
+irq_num = data_offset + index * 32;
+irq_mask = 1 << data_offset;
+if (s->sw_pending[index] & irq_mask) {
+extioi_update_irq(s, irq_num, 1);
+s->sw_pending[index] &= ~irq_mask;
+}
+old_data &= ~irq_mask;
+data_offset = ctz32(old_data);
+}
I'm still not convinced. Why would unmasking (enabling) an irq call update_irq, but
masking (disabling) the same irq not also call update_irq?
Even if that is correct, the testing of sw_pending could be done in parallel,
like
old_data &= s->sw_pending[index];
before the loop, instead of testing each bit one at a time within the loop.
The loop itself should be written
while (old_data) {
data_offset = ctz32(old_data);
...
old_data &= old_data - 1;
}
so that you don't bother computing ctz for zero values, and with the adjustment to
old_data before the loop, you don't need irq_mask within the loop.
Likewise with the updates to COREISR and COREMAP.
r~
[PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
This patch realize the EIOINTC interrupt controller.
Signed-off-by: Xiaojuan Yang
Signed-off-by: Song Gao
---
hw/intc/Kconfig| 3 +
hw/intc/loongarch_extioi.c | 325 +
hw/intc/meson.build| 1 +
hw/intc/trace-events | 5 +
hw/loongarch/Kconfig | 1 +
include/hw/intc/loongarch_extioi.h | 61 ++
6 files changed, 396 insertions(+)
create mode 100644 hw/intc/loongarch_extioi.c
create mode 100644 include/hw/intc/loongarch_extioi.h
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 58f550b865..ecd2883ceb 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -99,3 +99,6 @@ config LOONGARCH_PCH_MSI
select MSI_NONBROKEN
bool
select UNIMP
+
+config LOONGARCH_EXTIOI
+bool
diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
new file mode 100644
index 00..ddd6e15026
--- /dev/null
+++ b/hw/intc/loongarch_extioi.c
@@ -0,0 +1,325 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Loongson 3A5000 ext interrupt controller emulation
+ *
+ * Copyright (C) 2021 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/log.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+#include "hw/loongarch/virt.h"
+#include "hw/qdev-properties.h"
+#include "exec/address-spaces.h"
+#include "hw/intc/loongarch_extioi.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+
+static int get_coremap(LoongArchExtIOI *s, int irq_num)
+{
+int cpu;
+int cpu_index = irq_num / 4;
+int cpu_offset = irq_num & 0x3;
+int cpu_mask = 0xf << cpu_offset;
+
+cpu = (s->coremap[cpu_index] & cpu_mask) >> cpu_offset;
+cpu = ctz32(cpu);
+cpu = (cpu >= 4) ? 0 : cpu;
+return cpu;
+}
+
+static int get_ipmap(LoongArchExtIOI *s, int irq_num)
+{
+int ipnum;
+int ipmap_index = irq_num / 32 / 4;
+int ipmap_offset = (irq_num / 32) & 0x3;
+int ipmap_mask = 0xf << ipmap_offset;
+
+ipnum = (s->ipmap[ipmap_index] & ipmap_mask) >> ipmap_offset;
+ipnum = ctz32(ipnum);
+ipnum = (ipnum >= 4) ? 0 : ipnum;
+return ipnum;
+}
+
+static void extioi_update_irq(LoongArchExtIOI *s, int irq_num, int level)
+{
+int ipnum, cpu, found, irq_index, irq_mask;
+
+ipnum = get_ipmap(s, irq_num);
+cpu = get_coremap(s, irq_num);
+irq_index = irq_num / 32;
+irq_mask = 1 << (irq_num & 0x1f);
+
+if (level) {
+/* if not enable return false */
+if (((s->enable[irq_index]) & irq_mask) == 0) {
+s->sw_pending[irq_index] |= irq_mask;
+return;
+}
+s->coreisr[cpu][irq_index] |= irq_mask;
+found = find_first_bit(s->sw_isr[cpu][ipnum], EXTIOI_IRQS);
+set_bit(irq_num, s->sw_isr[cpu][ipnum]);
+if (found < EXTIOI_IRQS) {
+/* other irq is handling, need not update parent irq level */
+return;
+}
+} else {
+s->coreisr[cpu][irq_index] &= ~irq_mask;
+clear_bit(irq_num, s->sw_isr[cpu][ipnum]);
+found = find_first_bit(s->sw_isr[cpu][ipnum], EXTIOI_IRQS);
+if (found < EXTIOI_IRQS) {
+/* other irq is handling, need not update parent irq level */
+return;
+}
+}
+qemu_set_irq(s->parent_irq[cpu][ipnum], level);
+}
+
+static void extioi_setirq(void *opaque, int irq, int level)
+{
+LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
+trace_loongarch_extioi_setirq(irq, level);
+if (level) {
+s->isr[irq / 32] |= 1 << (irq % 32);
+} else {
+s->isr[irq / 32] &= ~(1 << (irq % 32));
+}
+extioi_update_irq(s, irq, level);
+}
+
+static uint64_t extioi_readw(void *opaque, hwaddr addr, unsigned size)
+{
+LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
+unsigned long offset = addr & 0x;
+uint32_t index, cpu, ret = 0;
+
+switch (offset) {
+case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
+index = (offset - EXTIOI_NODETYPE_START) >> 2;
+ret = s->nodetype[index];
+break;
+case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
+index = (offset - EXTIOI_IPMAP_START) >> 2;
+ret = s->ipmap[index];
+break;
+case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
+index = (offset - EXTIOI_ENABLE_START) >> 2;
+ret = s->enable[index];
+break;
+case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1:
+index = (offset - EXTIOI_BOUNCE_START) >> 2;
+ret = s->bounce[index];
+break;
+case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
+index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
+cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
+ret = s->coreisr[cpu][index];
+break;
+case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
+index = (offset - EXTIOI_COREMAP_START) >> 2;
+ret = s->coremap[index];
+break;
+default:
+
