qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: yangxiaojuan
Subject: Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
Date: Mon, 9 May 2022 17:38:08 +0800
User-agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

Hi Richard,

On 2022/5/7 下午11:31, Richard Henderson wrote:
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.

Thanks very much, I will consider the mapping format by read iocsr[0x420][49] like this:
static uint64_t map_format(void)
{
    LoongArchCPU *cpu;
    CPULoongArchState *env;
    uint64_t val;

    cpu = LOONGARCH_CPU(current_cpu);
    env = &(cpu->env);

    val = address_space_ldq(&env->address_space_iocsr, 0x420,
                             MEMTXATTRS_UNSPECIFIED, NULL);
    val &= 1 << 49;
    return val;
}
...
if (!map_format()) {
    cpu = ctz32(cpu);
    cpu = (cpu >= 4) ? 0 : cpu;
}
...
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.

Ok, there should be ISR registers(the status bitmap), i will add it to the LoongArchExtIOI, like this:
struct LoongArchExtIOI {
...
+    uint32_t isr[EXTIOI_IRQS / 32]
...
}

when extioi_setirq, update the isr filed.
static void extioi_setirq(void *opaque, int irq, int level)
{
    LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
    trace_loongarch_extioi_setirq(irq, level);
    s->isr[irq / 32] |= 1 << irq % 32;
    extioi_update_irq(s, irq, level);
}

and add ISR_START ... ISR_END to extioi_readw, like this
...
    case EXTIOI_ISR_START ... EXTIOI_ISR_END - 1:
        index = ((offset - EXTIOI_ISR_START) >> 2;
        ret = s->isr[index];
        break;
...


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?

Sorry, could you explain it in more detail? i can not understand the meanning of 'the entire interrupt map'? we only have ipmap and coremap registers in the LoongArchExtIOI, should we add an entire interrupt map?
+    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.

Thanks, i will add a condition to judge coreisr[cpu][index], excute extioi_update_irq when the coreisr val is 1, like this:

static int get_coremap(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;
    if (!map_format()) {
        cpu = ctz32(cpu);
        cpu = (cpu >= 4) ? 0 : cpu;
    }
    return cpu;
}

static int coreisr_level(LoongArchExtIOI *s, int irq_num)
{
    int cpu = get_coremap(irq_num);
    return s->coreisr[cpu][irq_num / 32];
}

...
             while (data_offset != 32) {
                 if (!(val & (1 << data_offset))) {
                    if (coreisr_level(s, data_offset + index * 32)) {
                        extioi_update_irq(s, data_offset + index * 32, 0);
                    }
                 }
...

BTW,  Could you help us to review  the patch [1]  or add some other reviewers ?

[1] :
[PATCH v3 40/43] hw/loongarch: Add LoongArch ls7a acpi device support

Thanks.
Xiaojuan




reply via email to

[Prev in Thread] Current Thread [Next in Thread]