Re: [RFC PATCH v7 19/29] hw/intc: Add LoongArch extioi interrupt controller(EIOINTC)

[yangxiaojuan]

On 2022/3/29 上午6:43, Richard Henderson wrote:
> On 3/28/22 06:57, Xiaojuan Yang wrote:
> > This patch realize the EIOINTC interrupt controller.
> >
> > Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> > Signed-off-by: Song Gao <gaosong@loongson.cn>
> > ---
> >   hw/intc/Kconfig                    |   3 +
> >   hw/intc/loongarch_extioi.c         | 408 +++++++++++++++++++++++++++++
> >   hw/intc/meson.build                |   1 +
> >   hw/intc/trace-events               |  11 +
> >   hw/loongarch/Kconfig               |   1 +
> >   include/hw/intc/loongarch_extioi.h |  77 ++++++
> >   6 files changed, 501 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 71c04c328e..28bd1f185d 100644
> > --- a/hw/intc/Kconfig
> > +++ b/hw/intc/Kconfig
> > @@ -96,3 +96,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..af28e8d6e9
> > --- /dev/null
> > +++ b/hw/intc/loongarch_extioi.c
> > @@ -0,0 +1,408 @@
> > +/* 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/loongarch.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(void *opaque, int irq_num, int level)
> > +{
> > +    LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
>
> I think this is not opaque anymore; you've already resolved it in the 
> caller.
> I think level should be 'bool'.
OK.
> > +    uint8_t  ipnum, cpu;
> > +    unsigned long found1, found2;
> > +
> > +    ipnum = s->sw_ipmap[irq_num];
> > +    cpu   = s->sw_coremap[irq_num];
> > +    if (level == 1) {
>
> Just if (level).
>
> > +        if (test_bit(irq_num, (void *)s->enable) == false) {
>
> This, and every other cast you're using for bitops.h functions, is 
> wrong.  You would need to declare these bitmaps properly as 'unsigned 
> long name[BITS_TO_LONGS(N)];'.
>
> That said, I would definitely use uint64_t, because that matches up 
> with the description of these registers in the manual.
we may not declare these bitmaps as 'unsigned long 
name[BITS_TO_LONGS(N)]. For example， ext_sw_ipisr 
sw_ipisr[MAX_CORES][LS3A_INTC_IP] is a two-dimensional array，
and it has a specific meaning, memregion options also restrict its size
> > +            return;
> > +        }
> > +        bitmap_set((void *)s->coreisr[cpu], irq_num, 1);
> > +        found1 = find_next_bit((void *)&(s->sw_ipisr[cpu][ipnum]),
> > +                               EXTIOI_IRQS, 0);
>
> find_next_bit with offset=0 is find_first_bit...
OK.
> > +        bitmap_set((void *)&(s->sw_ipisr[cpu][ipnum]), irq_num, 1);
> > +
> > +        if (found1 >= EXTIOI_IRQS) {
> > +            qemu_set_irq(s->parent_irq[cpu][ipnum], level);
> > +        }
>
> ... but what's the bitmap search doing?  It appears to be checking 
> that there are *no* bits set between 0 and EXTIOI_IRQS, and then 
> raising the irq if no bits set.  That seems wrong.
found1 >= EXTIOI_IRQS says there is no interrupt at present, then the 
new interrupt will be sent.
> > +    } else {
> > +        bitmap_clear((void *)s->coreisr[cpu], irq_num, 1);
> > +        found1 = find_next_bit((void *)&(s->sw_ipisr[cpu][ipnum]),
> > +                               EXTIOI_IRQS, 0);
> > +        bitmap_clear((void *)&(s->sw_ipisr[cpu][ipnum]), irq_num, 1);
> > +        found2 = find_next_bit((void *)&(s->sw_ipisr[cpu][ipnum]),
> > +                               EXTIOI_IRQS, 0);
> > +
> > +        if ((found1 < EXTIOI_IRQS) && (found2 >= EXTIOI_IRQS)) {
> > +            qemu_set_irq(s->parent_irq[cpu][ipnum], level);
> > +        }
> > +    }
> > +}
if ((found1 < EXTIOI_IRQS) && (found2 >= EXTIOI_IRQS)) says if the 
current interrupt number is equal to irq_num, then qemu_set_irq.
> It *seems* like all of this should be
>
>     uint64_t sum = 0;
>
>     s->isr[ipnum / 64] = deposit64(s->isr[ipnum / 64], ipnum % 64, 1, 
> level);
>
>     for (int i = 0; i < ARRAY_SIZE(s->isr); i++) {
>         sum |= s->isr[i] & s->ena[i];
>     }
>     qemu_set_irq(parent, sum != 0);
>
> If that's not the case, you need many more comments.
Yes, we need more comments,

Thanks.
Xiaojuan
> r~