Re: [PATCH 1/1] hw/intc/riscv_aplic: Fix APLIC in clrip and clripnum write emulation

[Yong-Xuan Wang]

Hi Daniel,

On Fri, Aug 9, 2024 at 5:40 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Ccing Anup
>
> On 8/8/24 5:20 AM, Yong-Xuan Wang wrote:
> > In the section "4.7 Precise effects on interrupt-pending bits"
> > of the RISC-V AIA specification defines that:
> >
> > If the source mode is Level1 or Level0 and the interrupt domain
> > is configured in MSI delivery mode (domaincfg.DM = 1):
> > The pending bit is cleared whenever the rectified input value is
> > low, when the interrupt is forwarded by MSI, or by a relevant
> > write to an in clrip register or to clripnum.
> >
> > Update the riscv_aplic_set_pending() to match the spec.
> >
>
> This would need a
>
> Fixes: bf31cf06eb ("hw/intc/riscv_aplic: Fix setipnum_le write emulation for 
> APLIC MSI-mode")
>

I will add it into next version. Thank you!

> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > ---
> >   hw/intc/riscv_aplic.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
> > index c1748c2d17d1..45d8b4089229 100644
> > --- a/hw/intc/riscv_aplic.c
> > +++ b/hw/intc/riscv_aplic.c
> > @@ -247,7 +247,7 @@ static void riscv_aplic_set_pending(RISCVAPLICState 
> > *aplic,
> >
> >       if ((sm == APLIC_SOURCECFG_SM_LEVEL_HIGH) ||
> >           (sm == APLIC_SOURCECFG_SM_LEVEL_LOW)) {
> > -        if (!aplic->msimode || (aplic->msimode && !pending)) {
> > +        if (!aplic->msimode) {
>
> The change you're doing here seems sensible to me but I'd rather have Anup 
> take
> a look to see if this somehow undo what was made here in commit bf31cf06.
>
> In particular w.r.t this paragraph from section 4.9.2 of AIA:
>
> "A second option is for the interrupt service routine to write the
> APLIC’s source identity number for the interrupt to the domain’s
> setipnum register just before exiting. This will cause the interrupt’s
> pending bit to be set to one again if the source is still asserting
> an interrupt, but not if the source is not asserting an interrupt."
>

I think that this patch won't affect setipnum. For the setipnum case,
the pending value would
be true. And it is handled by the 2 if conditions below.

Regards,
Yong-Xuan

>
> Thanks,
>
> Daniel
>
>
> >               return;
> >           }
> >           if ((aplic->state[irq] & APLIC_ISTATE_INPUT) &&