Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 2022/5/12 上午9:58, maobibo wrote: 在 2022/5/11 22:14, Richard Henderson 写道: On 5/11/22 02:54, yangxiaojuan wrote: On 2022/5/10 上午1:56, Richard Henderson wrote: + 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'? I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various cpus, as coreisr[*]. If either ipmap or coremap changes, do you need to re-compute coreisr[*] from the input isr[*]? I think the interrupt has been handled by the core before set coremap or ipmap, and coreisr[*] also has been set and cleard by original core. So, the new mapped core need not to update the coreisr[*]. Why do you believe that the core to which the interrupt is directed has interrupts enabled? Why do you believe the core to which the interrupt is directed is the one that is changing the interrupt mapping? I think there is no interrupt enable registers of percpu in extioi device. So, i think we need not to consider the core to which the interrupt is directed if has interrupts enabled. If the core to which the interrupt is directed is diffrent from the core that is changing the mapping, Should we copy the status value of coreisr[old_core][irq_num] to coreisr[new_core][irq_num]? Ip mapping could not affect coreisr[cpu][irq_num], Should we still need to update the interrupt? Thanks. xiaojuan By my understanding, irq bit of coreisr will be set even if the interrupt is disabled on the core, interrupt has been posted to core already, only that it is not serviced by the core. After irq affinity is changed, new interrupt may arrived to new core, one interrupt will be serviced by old core and new core at the same time. However it is the problem of guest kernel, guest kernel system should disable the irq and wait until irq finishes to be serviced on the old core when irq affinity is changing. I think your assumption is not correct. r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 2022/5/10 上午1:56, Richard Henderson wrote:
On 5/9/22 04:38, yangxiaojuan wrote:
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;
}
I'm not 100% sure how this "Other configuration control register"
should be handled, but definitely not like this.
Could we use the bitmapping as the default cpu or ip map format? Becaue
we emulate iocsr[0x420] as a default value, and it does not support to
write. We will add 'TOOD' logs and continue to modify them when using
other routing methods later.
What do you think of this idea？
Thanks.
Xiaojuan
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
在 2022/5/11 22:14, Richard Henderson 写道: > On 5/11/22 02:54, yangxiaojuan wrote: >> >> On 2022/5/10 上午1:56, Richard Henderson wrote: >>> >> + 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'? >>> >>> I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various >>> cpus, as coreisr[*]. If either ipmap or coremap changes, do you need to >>> re-compute coreisr[*] from the input isr[*]? >> >> I think the interrupt has been handled by the core before set coremap or >> ipmap, and coreisr[*] also has been set and cleard by original core. >> So, the new mapped core need not to update the coreisr[*]. > > > Why do you believe that the core to which the interrupt is directed has > interrupts enabled? Why do you believe the core to which the interrupt is > directed is the one that is changing the interrupt mapping? By my understanding, irq bit of coreisr will be set even if the interrupt is disabled on the core, interrupt has been posted to core already, only that it is not serviced by the core. After irq affinity is changed, new interrupt may arrived to new core, one interrupt will be serviced by old core and new core at the same time. However it is the problem of guest kernel, guest kernel system should disable the irq and wait until irq finishes to be serviced on the old core when irq affinity is changing. > > I think your assumption is not correct. > > > r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 5/11/22 02:54, yangxiaojuan wrote: On 2022/5/10 上午1:56, Richard Henderson wrote: + 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'? I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various cpus, as coreisr[*]. If either ipmap or coremap changes, do you need to re-compute coreisr[*] from the input isr[*]? I think the interrupt has been handled by the core before set coremap or ipmap, and coreisr[*] also has been set and cleard by original core. So, the new mapped core need not to update the coreisr[*]. Why do you believe that the core to which the interrupt is directed has interrupts enabled? Why do you believe the core to which the interrupt is directed is the one that is changing the interrupt mapping? I think your assumption is not correct. r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 2022/5/10 上午1:56, Richard Henderson wrote: + 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'? I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various cpus, as coreisr[*]. If either ipmap or coremap changes, do you need to re-compute coreisr[*] from the input isr[*]? I think the interrupt has been handled by the core before set coremap or ipmap, and coreisr[*] also has been set and cleard by original core. So, the new mapped core need not to update the coreisr[*]. Thanks. Xiaojuan
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
在 2022/5/10 11:56, Richard Henderson 写道: > On 5/9/22 19:54, maobibo wrote: >> how about put address_space_iocsr as board rather than percpu since there is >> no concept >> of "cpu package". > > "cpu package" works ok as a device on the board. > > I don't know if it's possible to have the iocsr address space controlled by > the device, especially since it appears that the iocsr address space is > *also* available -- or at least partially available -- in the main address > space at base 0x1fe0? In the current hw implementation, iocsr/mmio address space can both be used for IPI/EXTIOI, in future mmio address space will be deprecated and removed, only iocsr will be used since it is easy for hw to abstract the interface. In the next patch, main address space at base 0x1fe0 for IPI/EXTIOI will be removed and only iocsr address space will be used. And iocsr address space can controlled by device also. > > > r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 5/9/22 19:54, maobibo wrote: how about put address_space_iocsr as board rather than percpu since there is no concept of "cpu package". "cpu package" works ok as a device on the board. I don't know if it's possible to have the iocsr address space controlled by the device, especially since it appears that the iocsr address space is *also* available -- or at least partially available -- in the main address space at base 0x1fe0? r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
在 2022/5/10 10:54, maobibo 写道: > > > 在 2022/5/10 02:25, Richard Henderson 写道: >> On 5/9/22 13:04, Peter Maydell wrote: >>> On Mon, 9 May 2022 at 18:56, Richard Henderson >>> wrote: I'm not 100% sure how this "Other configuration control register" should be handled, but definitely not like this. I see you're putting control of this register into loongarch_qemu_read in target/loongarch/cpu.c. Which, I suppose is fair, because this is documented as part of the 3A5000 cpu documentation. But then you split out all of the devices which are *also* documented as part of the cpu into the board configuration. This reminds me of the memory-mapped interface that the armv7m cpu has with its own registers. I believe that you need to model this similarly, where you will have a device that represents the cpu, and then instantiates all of the devices that are listed in the Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for the 7A1000 bridge device. When there is a write to the ls3a "Other function configuration register", the ls3a will need to communicate the changes to the various bits to its various sub-devices. I do not think it unreasonable to have direct function calls between the components. Peter, do you have any advice from the armv7m side? >>> >>> Nothing concrete. I'm not sure that we'd structure the armv7m stuff the way >>> we have now if we were writing it from scratch, but it's functional enough. >>> (In particular, if MMIO regions were part of Device rather than SysBusDevice >>> then I'd be tempted to suggest that CPUs with MMIO-mapped registers should >>> directly own their MemoryRegions for them. But they aren't, so we can't do >>> that.) >> >> Having thought about this a little more, I believe that these registers are >> part of the "cpu package", because they are shared between the 4 cpu cores >> within the package. Thus their current placement attached to LoongArchCPU >> -- as well as the current placement of address_space_iocsr -- is incorrect. > > The extioi hardware design is not friend to software developer, local cpu INTC > is mixed with board INTC with extioi/iocsr. Local cpu INTC registers should > be banked, > address space is space for local cpu review point. address space of local cpu INTC should be the same from cpu viewpoint. > > how about put address_space_iocsr as board rather than percpu since there is > no concept > of "cpu package". > > regards > bibo, mao >> >> >> r~ >
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
在 2022/5/10 02:25, Richard Henderson 写道: > On 5/9/22 13:04, Peter Maydell wrote: >> On Mon, 9 May 2022 at 18:56, Richard Henderson >> wrote: >>> I'm not 100% sure how this "Other configuration control register" should be >>> handled, but >>> definitely not like this. >>> >>> I see you're putting control of this register into loongarch_qemu_read in >>> target/loongarch/cpu.c. Which, I suppose is fair, because this is >>> documented as part of >>> the 3A5000 cpu documentation. But then you split out all of the devices >>> which are *also* >>> documented as part of the cpu into the board configuration. >>> >>> This reminds me of the memory-mapped interface that the armv7m cpu has with >>> its own >>> registers. I believe that you need to model this similarly, where you will >>> have a device >>> that represents the cpu, and then instantiates all of the devices that are >>> listed in the >>> Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for >>> the 7A1000 >>> bridge device. >>> >>> When there is a write to the ls3a "Other function configuration register", >>> the ls3a will >>> need to communicate the changes to the various bits to its various >>> sub-devices. I do not >>> think it unreasonable to have direct function calls between the components. >>> >>> Peter, do you have any advice from the armv7m side? >> >> Nothing concrete. I'm not sure that we'd structure the armv7m stuff the way >> we have now if we were writing it from scratch, but it's functional enough. >> (In particular, if MMIO regions were part of Device rather than SysBusDevice >> then I'd be tempted to suggest that CPUs with MMIO-mapped registers should >> directly own their MemoryRegions for them. But they aren't, so we can't do >> that.) > > Having thought about this a little more, I believe that these registers are > part of the "cpu package", because they are shared between the 4 cpu cores > within the package. Thus their current placement attached to LoongArchCPU -- > as well as the current placement of address_space_iocsr -- is incorrect. The extioi hardware design is not friend to software developer, local cpu INTC is mixed with board INTC with extioi/iocsr. Local cpu INTC registers should be banked, address space is space for local cpu review point. how about put address_space_iocsr as board rather than percpu since there is no concept of "cpu package". regards bibo, mao > > > r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 5/9/22 13:04, Peter Maydell wrote: On Mon, 9 May 2022 at 18:56, Richard Henderson wrote: I'm not 100% sure how this "Other configuration control register" should be handled, but definitely not like this. I see you're putting control of this register into loongarch_qemu_read in target/loongarch/cpu.c. Which, I suppose is fair, because this is documented as part of the 3A5000 cpu documentation. But then you split out all of the devices which are *also* documented as part of the cpu into the board configuration. This reminds me of the memory-mapped interface that the armv7m cpu has with its own registers. I believe that you need to model this similarly, where you will have a device that represents the cpu, and then instantiates all of the devices that are listed in the Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for the 7A1000 bridge device. When there is a write to the ls3a "Other function configuration register", the ls3a will need to communicate the changes to the various bits to its various sub-devices. I do not think it unreasonable to have direct function calls between the components. Peter, do you have any advice from the armv7m side? Nothing concrete. I'm not sure that we'd structure the armv7m stuff the way we have now if we were writing it from scratch, but it's functional enough. (In particular, if MMIO regions were part of Device rather than SysBusDevice then I'd be tempted to suggest that CPUs with MMIO-mapped registers should directly own their MemoryRegions for them. But they aren't, so we can't do that.) Having thought about this a little more, I believe that these registers are part of the "cpu package", because they are shared between the 4 cpu cores within the package. Thus their current placement attached to LoongArchCPU -- as well as the current placement of address_space_iocsr -- is incorrect. r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On Mon, 9 May 2022 at 18:56, Richard Henderson wrote: > I'm not 100% sure how this "Other configuration control register" should be > handled, but > definitely not like this. > > I see you're putting control of this register into loongarch_qemu_read in > target/loongarch/cpu.c. Which, I suppose is fair, because this is documented > as part of > the 3A5000 cpu documentation. But then you split out all of the devices > which are *also* > documented as part of the cpu into the board configuration. > > This reminds me of the memory-mapped interface that the armv7m cpu has with > its own > registers. I believe that you need to model this similarly, where you will > have a device > that represents the cpu, and then instantiates all of the devices that are > listed in the > Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for the > 7A1000 > bridge device. > > When there is a write to the ls3a "Other function configuration register", > the ls3a will > need to communicate the changes to the various bits to its various > sub-devices. I do not > think it unreasonable to have direct function calls between the components. > > Peter, do you have any advice from the armv7m side? Nothing concrete. I'm not sure that we'd structure the armv7m stuff the way we have now if we were writing it from scratch, but it's functional enough. (In particular, if MMIO regions were part of Device rather than SysBusDevice then I'd be tempted to suggest that CPUs with MMIO-mapped registers should directly own their MemoryRegions for them. But they aren't, so we can't do that.) -- PMM
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 5/9/22 04:38, yangxiaojuan wrote:
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;
}
I'm not 100% sure how this "Other configuration control register" should be handled, but
definitely not like this.
I see you're putting control of this register into loongarch_qemu_read in
target/loongarch/cpu.c. Which, I suppose is fair, because this is documented as part of
the 3A5000 cpu documentation. But then you split out all of the devices which are *also*
documented as part of the cpu into the board configuration.
This reminds me of the memory-mapped interface that the armv7m cpu has with its own
registers. I believe that you need to model this similarly, where you will have a device
that represents the cpu, and then instantiates all of the devices that are listed in the
Loongson 3A5000 TRM -- call this ls3a to match the ls7a name you have for the 7A1000
bridge device.
When there is a write to the ls3a "Other function configuration register", the ls3a will
need to communicate the changes to the various bits to its various sub-devices. I do not
think it unreasonable to have direct function calls between the components.
Peter, do you have any advice from the armv7m side?
+ 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'?
I mean, ipmap[*] and coremap[*] controls how isr[*] maps to the various cpus, as
coreisr[*]. If either ipmap or coremap changes, do you need to re-compute coreisr[*] from
the input isr[*]?
r~
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
On 2022/5/7 下午11:31, Richard Henderson wrote:
+ 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.
Thanks, i should consider the MAP registers overlap outputs.
And i want to add 'uint32_t sw_isr_group[256 / 32]', when each bit of
sw_isr_group[n*32+31 : n*32] is 0, then lower the level of the cpu pin.
Thanks.
Xiaojuan
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
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 & 0x;
+
+ 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_offse
Re: [PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
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 & 0x;
+
+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~
[PATCH v3 34/43] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)
This patch realize the EIOINTC interrupt controller.
Signed-off-by: Xiaojuan Yang
Signed-off-by: Song Gao
---
hw/intc/Kconfig| 3 +
hw/intc/loongarch_extioi.c | 248 +
hw/intc/meson.build| 1 +
hw/intc/trace-events | 5 +
hw/loongarch/Kconfig | 1 +
include/hw/intc/loongarch_extioi.h | 58 +++
6 files changed, 316 insertions(+)
create mode 100644 hw/intc/loongarch_extioi.c
create mode 100644 include/hw/intc/loongarch_extioi.h
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 58f550b865..ecd2883ceb 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -99,3 +99,6 @@ config LOONGARCH_PCH_MSI
select MSI_NONBROKEN
bool
select UNIMP
+
+config LOONGARCH_EXTIOI
+bool
diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
new file mode 100644
index 00..6b49123512
--- /dev/null
+++ b/hw/intc/loongarch_extioi.c
@@ -0,0 +1,248 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Loongson 3A5000 ext interrupt controller emulation
+ *
+ * Copyright (C) 2021 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/log.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+#include "hw/loongarch/virt.h"
+#include "hw/qdev-properties.h"
+#include "exec/address-spaces.h"
+#include "hw/intc/loongarch_extioi.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+static void extioi_update_irq(LoongArchExtIOI *s, int irq_num, int level)
+{
+int ipnum, cpu;
+
+int ipmap_index = irq_num / 32 / 4;
+int ipmap_offset = (irq_num / 32) & 0x3;
+int ipmap_mask = 0xff << ipmap_offset;
+
+int cpu_index = irq_num / 4;
+int cpu_offset = irq_num & 0x3;
+int cpu_mask = 0xff << ipmap_offset;
+
+int coreisr_index = irq_num / 32;
+int enable_index = coreisr_index;
+int coreisr_mask = irq_num & 0x1f;
+int enable_mask = coreisr_mask;
+/*
+ * Routing in group of 32 interrupts.
+ * The default value of csr[0x420][49]
+ * is 0 and nobody will change it,
+ * so 'ipmap' use bitmap function.
+ */
+
+ipnum = ((s->ipmap[ipmap_index] & ipmap_mask) >> ipmap_offset) & 0xf;
+ipnum = ctz32(ipnum);
+ipnum = (ipnum >= 4) ? 0 : ipnum;
+
+cpu = ((s->coremap[cpu_index] & cpu_mask) >> cpu_offset) & 0xf;
+cpu = ctz32(cpu);
+cpu = (cpu >= 4) ? 0 : cpu;
+
+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);
+}
+}
+
+static void extioi_setirq(void *opaque, int irq, int level)
+{
+LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
+trace_loongarch_extioi_setirq(irq, level);
+extioi_update_irq(s, irq, level);
+}
+
+static uint64_t extioi_readw(void *opaque, hwaddr addr, unsigned size)
+{
+LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
+unsigned long offset = addr & 0x;
+uint32_t index, cpu, ret = 0;
+
+switch (offset) {
+case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
+index = (offset - EXTIOI_NODETYPE_START) >> 2;
+ret = s->nodetype[index];
+break;
+case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
+index = (offset - EXTIOI_IPMAP_START) >> 2;
+ret = s->ipmap[index];
+break;
+case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
+index = (offset - EXTIOI_ENABLE_START) >> 2;
+ret = s->enable[index];
+break;
+case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1:
+index = (offset - EXTIOI_BOUNCE_START) >> 2;
+ret = s->bounce[index];
+break;
+case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
+index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
+cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
+ret = s->coreisr[cpu][index];
+break;
+case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
+index = (offset - EXTIOI_COREMAP_START) >> 2;
+ret = s->coremap[index];
+break;
+default:
+break;
+}
+
+trace_loongarch_extioi_readw((uint32_t)addr, ret);
+return ret;
+}
+
+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 & 0x;
+
+switch (offset) {
+case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
+index = (offset - EXTIOI_NODETYPE_START) >> 2;
+s->nodetype[index] = val;
+
