Re: [PATCH 1/1] hw/intc/riscv_aplic: Check and update pending when write sourcecfg

[Yong-Xuan Wang <yongxuan.wang@sifive.com>]

Hi Alistair,

On Mon, Sep 9, 2024 at 10:32 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 6:21 PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> >
> > The section 4.5.2 of the RISC-V AIA specification says that any write
> > to a sourcecfg register of an APLIC might (or might not) cause the
> > corresponding interrupt-pending bit to be set to one if the rectified
> > input value is high (= 1) under the new source mode.
> >
> > If an interrupt is asserted before the driver configs its interrupt
> > type to APLIC, it's pending bit will not be set except a relevant
> > write to a setip or setipnum register. When we write the interrupt
> > type to sourcecfg register, if the APLIC device doesn't check
> > rectified input value and update the pending bit, this interrupt
> > might never becomes pending.
> >
> > For APLIC.m, we can manully set pending by setip or setipnum
> > registers in driver. But for APLIC.w, the pending status totally
> > depends on the rectified input value, we can't control the pending
> > status via mmio registers. In this case, hw should check and update
> > pending status for us when writing sourcecfg registers.
> >
> > Update QEMU emulation to handle "pre-existing" interrupts.
> >
> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
>
> > ---
> >  hw/intc/riscv_aplic.c | 49 +++++++++++++++++++++++++++----------------
> >  1 file changed, 31 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> > index 32edd6d07bb3..2a9ac76ce92e 100644
> > --- a/hw/intc/riscv_aplic.c
> > +++ b/hw/intc/riscv_aplic.c
> > @@ -159,31 +159,41 @@ static bool is_kvm_aia(bool msimode)
> >      return kvm_irqchip_in_kernel() && msimode;
> >  }
> >
> > +static bool riscv_aplic_irq_rectified_val(RISCVAPLICState *aplic,
> > +                                          uint32_t irq)
> > +{
> > +    uint32_t sourcecfg, sm, raw_input, irq_inverted;
> > +
> > +    if (!irq || aplic->num_irqs <= irq) {
> > +        return false;
> > +    }
> > +
> > +    sourcecfg = aplic->sourcecfg[irq];
> > +    if (sourcecfg & APLIC_SOURCECFG_D) {
> > +        return false;
> > +    }
> > +
> > +    sm = sourcecfg & APLIC_SOURCECFG_SM_MASK;
> > +    if (sm == APLIC_SOURCECFG_SM_INACTIVE) {
> > +        return false;
> > +    }
> > +
> > +    raw_input = (aplic->state[irq] & APLIC_ISTATE_INPUT) ? 1 : 0;
> > +    irq_inverted = (sm == APLIC_SOURCECFG_SM_LEVEL_LOW ||
> > +                    sm == APLIC_SOURCECFG_SM_EDGE_FALL) ? 1 : 0;
> > +    return !!(raw_input ^ irq_inverted);
> > +}
> > +
> >  static uint32_t riscv_aplic_read_input_word(RISCVAPLICState *aplic,
> >                                              uint32_t word)
> >  {
> > -    uint32_t i, irq, sourcecfg, sm, raw_input, irq_inverted, ret = 0;
> > +    uint32_t i, irq, rectified_val, ret = 0;
> >
> >      for (i = 0; i < 32; i++) {
> >          irq = word * 32 + i;
> > -        if (!irq || aplic->num_irqs <= irq) {
> > -            continue;
> > -        }
> > -
> > -        sourcecfg = aplic->sourcecfg[irq];
> > -        if (sourcecfg & APLIC_SOURCECFG_D) {
> > -            continue;
> > -        }
> > -
> > -        sm = sourcecfg & APLIC_SOURCECFG_SM_MASK;
> > -        if (sm == APLIC_SOURCECFG_SM_INACTIVE) {
> > -            continue;
> > -        }
> >
> > -        raw_input = (aplic->state[irq] & APLIC_ISTATE_INPUT) ? 1 : 0;
> > -        irq_inverted = (sm == APLIC_SOURCECFG_SM_LEVEL_LOW ||
> > -                        sm == APLIC_SOURCECFG_SM_EDGE_FALL) ? 1 : 0;
> > -        ret |= (raw_input ^ irq_inverted) << i;
> > +        rectified_val = riscv_aplic_irq_rectified_val(aplic, irq);
> > +        ret |= rectified_val << i;
> >      }
> >
> >      return ret;
> > @@ -702,6 +712,9 @@ static void riscv_aplic_write(void *opaque, hwaddr addr, uint64_t value,
> >              (aplic->sourcecfg[irq] == 0)) {
> >              riscv_aplic_set_pending_raw(aplic, irq, false);
> >              riscv_aplic_set_enabled_raw(aplic, irq, false);
> > +        } else {
> > +            if (riscv_aplic_irq_rectified_val(aplic, irq))
> > +                riscv_aplic_set_pending_raw(aplic, irq, true);
>
> You need curly braces for the if statement. You can run checkpatch.pl
> to catch issues like this
>

Thank you! I will fix it.

Regards,
Yong-Xuan

> Alistair
>
> >          }
> >      } else if (aplic->mmode && aplic->msimode &&
> >                 (addr == APLIC_MMSICFGADDR)) {
> > --
> > 2.17.1
> >
> >