[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(
From: |
Richard Henderson |
Subject: |
Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC) |
Date: |
Wed, 18 May 2022 11:04:20 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 |
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 & 0xffff;
+
+ 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 19/43] target/loongarch: Add CSRs definition, (continued)
- [PATCH v4 19/43] target/loongarch: Add CSRs definition, Xiaojuan Yang, 2022/05/17
- [PATCH v4 28/43] target/loongarch: Add other core instructions support, Xiaojuan Yang, 2022/05/17
- [PATCH v4 30/43] hw/loongarch: Add support loongson3 virt machine type., Xiaojuan Yang, 2022/05/17
- [PATCH v4 32/43] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC), Xiaojuan Yang, 2022/05/17
- [PATCH v4 42/43] tests/tcg/loongarch64: Add hello/memory test in loongarch64 system, Xiaojuan Yang, 2022/05/17
- [PATCH v4 23/43] target/loongarch: Add LoongArch interrupt and exception handle, Xiaojuan Yang, 2022/05/17
- [PATCH v4 20/43] target/loongarch: Add basic vmstate description of CPU., Xiaojuan Yang, 2022/05/17
- [PATCH v4 35/43] hw/loongarch: Add irq hierarchy for the system, Xiaojuan Yang, 2022/05/17
- [PATCH v4 37/43] hw/loongarch: Add some devices support for 3A5000., Xiaojuan Yang, 2022/05/17
- [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC), Xiaojuan Yang, 2022/05/17
- Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC),
Richard Henderson <=
- [PATCH v4 39/43] hw/loongarch: Add LoongArch load elf function., Xiaojuan Yang, 2022/05/17
- [PATCH v4 41/43] target/loongarch: Add gdb support., Xiaojuan Yang, 2022/05/17
- [PATCH v4 38/43] hw/loongarch: Add LoongArch ls7a rtc device support, Xiaojuan Yang, 2022/05/17
[PATCH v4 40/43] hw/loongarch: Add LoongArch ls7a acpi device support, Xiaojuan Yang, 2022/05/17