| Message ID | 20240808081948.25837-1-yongxuan.wang@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] hw/intc/riscv_aplic: Check and update pending when write sourcecfg | expand |
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 Alistair > } > } else if (aplic->mmode && aplic->msimode && > (addr == APLIC_MMSICFGADDR)) { > -- > 2.17.1 > >
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 > > > >
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); } } else if (aplic->mmode && aplic->msimode && (addr == APLIC_MMSICFGADDR)) {
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> --- hw/intc/riscv_aplic.c | 49 +++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 18 deletions(-)