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
