Hi Alistair,
On Mon, Sep 9, 2024 at 10:32 AM Alistair Francis <alistai...@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 6:21 PM Yong-Xuan Wang <yongxuan.w...@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.w...@sifive.com>
>
> Acked-by: Alistair Francis <alistair.fran...@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
> >
> >
