| Message ID | 20220429100729.1572481-35-yangxiaojuan@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add LoongArch softmmu support | expand |
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~
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
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
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~
On Mon, 9 May 2022 at 18:56, Richard Henderson <richard.henderson@linaro.org> 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
On 5/9/22 13:04, Peter Maydell wrote: > On Mon, 9 May 2022 at 18:56, Richard Henderson > <richard.henderson@linaro.org> 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~
在 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 >> <richard.henderson@linaro.org> 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~
在 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 >>> <richard.henderson@linaro.org> 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~ >
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 0x1fe00000? r~
在 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 0x1fe00000? 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 0x1fe00000 for IPI/EXTIOI will be removed and only iocsr address space will be used. And iocsr address space can controlled by device also. > > > r~
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
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~
在 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~
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
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~
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 0000000000..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 & 0xffff; + 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 & 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; + 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); + } + old_data &= ~(1 << data_offset); + data_offset = ctz32(old_data); + } + } + break; + case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1: + index = (offset - EXTIOI_BOUNCE_START) >> 2; + s->bounce[index] = val; + break; + case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1: + index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2; + cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3; + old_data = s->coreisr[cpu][index]; + s->coreisr[cpu][index] = old_data & ~val; + if (old_data != s->coreisr[cpu][index]) { + data_offset = ctz32(val); + while (data_offset != 32) { + if ((old_data & (1 << data_offset))) { + extioi_update_irq(s, data_offset + index * 32, 0); + } + val &= ~(1 << data_offset); + data_offset = ctz32(val); + } + } + break; + case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1: + index = (offset - EXTIOI_COREMAP_START) >> 2; + s->coremap[index] = val; + break; + + default: + break; + } +} + +static const MemoryRegionOps extioi_ops = { + .read = extioi_readw, + .write = extioi_writew, + .impl.min_access_size = 4, + .impl.max_access_size = 4, + .valid.min_access_size = 4, + .valid.max_access_size = 8, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + +static const VMStateDescription vmstate_loongarch_extioi = { + .name = TYPE_LOONGARCH_EXTIOI, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(bounce, LoongArchExtIOI, EXTIOI_IRQS_GROUP_COUNT), + VMSTATE_UINT32_2DARRAY(coreisr, LoongArchExtIOI, LOONGARCH_MAX_VCPUS, + EXTIOI_IRQS_GROUP_COUNT), + VMSTATE_UINT32_ARRAY(nodetype, LoongArchExtIOI, + EXTIOI_IRQS_NODETYPE_COUNT / 2), + VMSTATE_UINT32_ARRAY(enable, LoongArchExtIOI, 8), + VMSTATE_UINT32_ARRAY(ipmap, LoongArchExtIOI, 2), + VMSTATE_UINT32_ARRAY(coremap, LoongArchExtIOI, EXTIOI_IRQS / 4), + VMSTATE_END_OF_LIST() + } +}; + +static void loongarch_extioi_instance_init(Object *obj) +{ + SysBusDevice *dev = SYS_BUS_DEVICE(obj); + LoongArchExtIOI *s = LOONGARCH_EXTIOI(obj); + int i, cpu, pin; + + for (i = 0; i < EXTIOI_IRQS; i++) { + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[i]); + } + + qdev_init_gpio_in(DEVICE(obj), extioi_setirq, EXTIOI_IRQS); + + for (cpu = 0; cpu < LOONGARCH_MAX_VCPUS; cpu++) { + memory_region_init_io(&s->extioi_iocsr_mem[cpu], OBJECT(s), &extioi_ops, + s, "extioi_iocsr", 0x900); + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->extioi_iocsr_mem[cpu]); + for (pin = 0; pin < LS3A_INTC_IP; pin++) { + qdev_init_gpio_out(DEVICE(obj), &s->parent_irq[cpu][pin], 1); + } + } + memory_region_init_io(&s->extioi_system_mem, OBJECT(s), &extioi_ops, + s, "extioi_system_mem", 0x900); + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->extioi_system_mem); +} + +static void loongarch_extioi_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &vmstate_loongarch_extioi; +} + +static const TypeInfo loongarch_extioi_info = { + .name = TYPE_LOONGARCH_EXTIOI, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_init = loongarch_extioi_instance_init, + .instance_size = sizeof(struct LoongArchExtIOI), + .class_init = loongarch_extioi_class_init, +}; + +static void loongarch_extioi_register_types(void) +{ + type_register_static(&loongarch_extioi_info); +} + +type_init(loongarch_extioi_register_types) diff --git a/hw/intc/meson.build b/hw/intc/meson.build index 1d407c046d..bcbf22ff51 100644 --- a/hw/intc/meson.build +++ b/hw/intc/meson.build @@ -66,3 +66,4 @@ specific_ss.add(when: 'CONFIG_NIOS2_VIC', if_true: files('nios2_vic.c')) specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c')) specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c')) specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_MSI', if_true: files('loongarch_pch_msi.c')) +specific_ss.add(when: 'CONFIG_LOONGARCH_EXTIOI', if_true: files('loongarch_extioi.c')) diff --git a/hw/intc/trace-events b/hw/intc/trace-events index 8bcc1b6992..2069cda51d 100644 --- a/hw/intc/trace-events +++ b/hw/intc/trace-events @@ -303,3 +303,8 @@ loongarch_pch_pic_writeb(unsigned size, uint32_t addr, unsigned long val) "size: # loongarch_pch_msi.c loongarch_msi_set_irq(int irq_num) "set msi irq %d" + +# loongarch_extioi.c +loongarch_extioi_setirq(int irq, int level) "set extirq irq %d level %d" +loongarch_extioi_readw(uint32_t addr, uint32_t val) "addr: 0x%"PRIx32 "val: 0x%" PRIx32 +loongarch_extioi_writew(unsigned size, uint32_t addr, uint32_t val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx32 diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig index d814fc6103..f779087416 100644 --- a/hw/loongarch/Kconfig +++ b/hw/loongarch/Kconfig @@ -5,3 +5,4 @@ config LOONGARCH_VIRT select LOONGARCH_IPI select LOONGARCH_PCH_PIC select LOONGARCH_PCH_MSI + select LOONGARCH_EXTIOI diff --git a/include/hw/intc/loongarch_extioi.h b/include/hw/intc/loongarch_extioi.h new file mode 100644 index 0000000000..b7057bbc34 --- /dev/null +++ b/include/hw/intc/loongarch_extioi.h @@ -0,0 +1,58 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * LoongArch 3A5000 ext interrupt controller definitions + * + * Copyright (C) 2021 Loongson Technology Corporation Limited + */ + +#include "hw/sysbus.h" +#include "hw/loongarch/virt.h" + +#ifndef LOONGARCH_EXTIOI_H +#define LOONGARCH_EXTIOI_H + +#define LS3A_INTC_IP 8 +#define EXTIOI_IRQS (256) +#define EXTIOI_IRQS_BITMAP_SIZE (256 / 8) +/* map to ipnum per 32 irqs */ +#define EXTIOI_IRQS_IPMAP_SIZE (256 / 32) +#define EXTIOI_IRQS_COREMAP_SIZE 256 +#define EXTIOI_IRQS_NODETYPE_COUNT 16 +#define EXTIOI_IRQS_GROUP_COUNT 8 + +#define APIC_OFFSET 0x400 +#define APIC_BASE (0x1000ULL + APIC_OFFSET) + +#define EXTIOI_NODETYPE_START (0x4a0 - APIC_OFFSET) +#define EXTIOI_NODETYPE_END (0x4c0 - APIC_OFFSET) +#define EXTIOI_IPMAP_START (0x4c0 - APIC_OFFSET) +#define EXTIOI_IPMAP_END (0x4c8 - APIC_OFFSET) +#define EXTIOI_ENABLE_START (0x600 - APIC_OFFSET) +#define EXTIOI_ENABLE_END (0x620 - APIC_OFFSET) +#define EXTIOI_BOUNCE_START (0x680 - APIC_OFFSET) +#define EXTIOI_BOUNCE_END (0x6a0 - APIC_OFFSET) +#define EXTIOI_ISR_START (0x700 - APIC_OFFSET) +#define EXTIOI_ISR_END (0x720 - APIC_OFFSET) +#define EXTIOI_COREISR_START (0x800 - APIC_OFFSET) +#define EXTIOI_COREISR_END (0xB20 - APIC_OFFSET) +#define EXTIOI_COREMAP_START (0xC00 - APIC_OFFSET) +#define EXTIOI_COREMAP_END (0xD00 - APIC_OFFSET) + +#define EXTIOI_SYSTEM_MEM 0x1fe01400 +#define TYPE_LOONGARCH_EXTIOI "loongarch.extioi" +OBJECT_DECLARE_SIMPLE_TYPE(LoongArchExtIOI, LOONGARCH_EXTIOI) +struct LoongArchExtIOI { + SysBusDevice parent_obj; + /* hardware state */ + uint32_t nodetype[EXTIOI_IRQS_NODETYPE_COUNT / 2]; + uint32_t bounce[EXTIOI_IRQS_GROUP_COUNT]; + uint32_t coreisr[LOONGARCH_MAX_VCPUS][EXTIOI_IRQS_GROUP_COUNT]; + uint32_t enable[8]; + uint32_t ipmap[2]; + uint32_t coremap[EXTIOI_IRQS / 4]; + qemu_irq parent_irq[LOONGARCH_MAX_VCPUS][LS3A_INTC_IP]; + qemu_irq irq[EXTIOI_IRQS]; + MemoryRegion extioi_iocsr_mem[LOONGARCH_MAX_VCPUS]; + MemoryRegion extioi_system_mem; +}; +#endif /* LOONGARCH_EXTIOI_H */