Re: [PATCH v1 33/43] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

[Richard Henderson]

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.


r~