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

[Richard Henderson]

On 4/29/22 05:07, Xiaojuan Yang wrote:
> +    int ipmap_mask = 0xff << ipmap_offset;
...
> +    int cpu_mask = 0xff << ipmap_offset;

These two masks are redundant with

> +    ipnum = ((s->ipmap[ipmap_index] & ipmap_mask) >> ipmap_offset) & 0xf;
...
> +    cpu = ((s->coremap[cpu_index] & cpu_mask) >> cpu_offset) & 0xf;

the 0xf masking here.

> +    cpu = ctz32(cpu);
> +    cpu = (cpu >= 4) ? 0 : cpu;

You are not considering CSR[0x420][49], which changes the format of this 
mapping.

I think this function is wrong because you maintain an unmapped enable bitmap, but you do 
not maintain an unmapped status bitmap, which *should* be readable from 
EXTIOI_ISR_{START,END}, but is not present in extioi_readw.

I think that only extioi_setirq should actually change the unmapped status bitmap, and 
that extioi_update_irq should only evaluate the mapping to apply changes to the cpus.


> +    if (level) {
> +        /* if not enable return false */
> +        if (((s->enable[enable_index]) & (1 << enable_mask)) == 0) {
> +            return;
> +        }
> +        s->coreisr[cpu][coreisr_index] |= (1 << coreisr_mask);
> +        qemu_set_irq(s->parent_irq[cpu][ipnum], level);
> +    } else {
> +        s->coreisr[cpu][coreisr_index] &= ~(1 << coreisr_mask);
> +        qemu_set_irq(s->parent_irq[cpu][ipnum], level);
> +    }

This final bit, updating the cpu irq is also wrong, in that it should be unconditional. 
This is the only way that it will work for the usage in updating the enable mask.

I think you are not considering when the MAP registers overlap outputs.  For instance, if 
all 256 bits of EXT_IOIMap contain 0, then all of EXT_IOI[n*32+31 : n*32] overlap.  When 
that happens, you cannot lower the level of the cpu pin until all of the matching ioi 
interrupts are low.

Or, perhaps I don't understand how this is supposed to work?
The documentation is very weak.


> +static void extioi_writew(void *opaque, hwaddr addr,
> +                                   uint64_t val, unsigned size)
> +{
> +    LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
> +    int cpu, index, old_data, data_offset;
> +    uint32_t offset;
> +    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:
> +        index = (offset - EXTIOI_IPMAP_START) >> 2;
> +        s->ipmap[index] = val;
> +        break;

Do you need to recompute the entire interrupt map when ipmap changes?

> +    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;
> +            old_data = old_data ^ val;
> +            data_offset = ctz32(old_data);
> +            while (data_offset != 32) {
> +                if (!(val & (1 << data_offset))) {
> +                    extioi_update_irq(s, data_offset + index * 32, 0);

This is not correct -- you're unconditionally setting level=0, corrupting the old value of 
coreisr[cpu][index].  You need to recompute *without* changning those levels.

> +    case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
> +        index = (offset - EXTIOI_COREMAP_START) >> 2;
> +        s->coremap[index] = val;
> +        break;

Recompute the entire interrupt map?


r~