On 2022/4/18 下午10:39, Richard Henderson wrote:
On 4/18/22 02:14, yangxiaojuan wrote:
Hi, Richard
On 2022/4/18 上午11:15, Richard Henderson wrote:
On 4/15/22 02:40, Xiaojuan Yang wrote:
+static void pch_pic_update_irq(LoongArchPCHPIC *s, uint32_t mask,
+ int level, int hi)
+{
+ uint32_t val, irq;
+
+ if (level == 1) {
+ if (hi) {
+ val = mask & s->intirr_hi & (~s->int_mask_hi);
+ irq = find_first_bit((unsigned long *)&val, 32);
This does not work -- you're accessing beyond the end of the
uint32_t for all LP64 hosts. I think you just want ctz32()...
+ if (irq != 32) {
+ s->intisr_hi |= 1ULL << irq;
+ qemu_set_irq(s->parent_irq[s->htmsi_vector[irq + 32]], 1);
+ }
... which should in fact only be tested if val != 0, which is to
what this IF equates.
Is there a good reason that this function is treating hi and lo
separately, as opposed to simply doing all of the computation on
uint64_t?
In the part of linux kernel, pch pic driver use 32 bits for reading
and writing.
e.g in the drivers/irqchip/irq-loongson-pch-pic.c， pch_pic_mask_irq()
function use writel() to write pch_pic mask reg.
So? update_irq is not writel.
I expect that you should have done something (manual deposit64 or
.impl.min_access_size = 8) with the actual writel, but by the time we
arrive in this subroutine we have a complete uint64_t.
In the linux kernel pch_pic driver, pch_pic_unmask_irq() use writel to
config PCH_PIC_CLR reg:
writel(BIT(PIC_REG_BIT(d->hwirq)),
priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
And writel() need u32 value.
static inline void writel(u32 value, volatile void __iomem *addr)
{
__io_bw();
__raw_writel((u32 __force)__cpu_to_le32(value), addr);
__io_aw();
}
The emulate of PCH_PIC_CLR in qemu LoongArchPCHPIC struct member is
intirr_lo/hi(we devide 64bits reg to two 32bits reg to match the linux
kernel), it will be changed when we config clear reg or handler irq.
static void loongarch_pch_pic_low_writew(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
...
case PCH_PIC_INT_CLEAR_LO:
if (s->intedge_lo & data) {
s->intirr_lo &= (~data);
pch_pic_update_irq(s, data, 0, 0);
s->intisr_lo &= (~data);
}
break;
case PCH_PIC_INT_CLEAR_HI:
if (s->intedge_hi & data) {
s->intirr_hi &= (~data);
pch_pic_update_irq(s, data, 0, 1);
s->intisr_hi &= (~data);
}
break;
...
}
static void pch_pic_irq_handler(void *opaque, int irq, int level)
{
...
hi = (irq >= 32) ? 1 : 0;
if (hi) {
irq = irq - 32;
}
mask = 1ULL << irq;
if (hi) {
if (s->intedge_hi & mask) {
/* Edge triggered */
if (level) {
if ((s->last_intirr_hi & mask) == 0) {
s->intirr_hi |= mask;
}
s->last_intirr_hi |= mask;
} else {
s->last_intirr_hi &= ~mask;
}
...
pch_pic_update_irq(s, mask, level, hi);
}
And in update_irq(), we should judge intirr，which irq number is pending.
so we also need whether is intirr_lo or intirr_hi.
static void pch_pic_update_irq(LoongArchPCHPIC *s, uint32_t mask,
int level, int hi)
{
uint32_t val, irq;
if (level) {
if (hi) {
val = mask & s->intirr_hi & (~s->int_mask_hi);
if (val) {
irq = ctz32(val);
s->intisr_hi |= 1ULL << irq;
qemu_set_irq(s->parent_irq[s->htmsi_vector[irq + 32]], 1);
}
......
}
Thanks.
Xiaojuan
r~
