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

[yangxiaojuan]

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~
>       
>