Re: [PATCH v4 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)

[Richard Henderson <richard.henderson@linaro.org>]

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~