diff mbox series

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

Message ID 20240808082030.25940-1-yongxuan.wang@sifive.com
State New
Headers show
Series [1/1] hw/intc/riscv_aplic: Fix APLIC in clrip and clripnum write emulation | expand

Commit Message

Yong-Xuan Wang Aug. 8, 2024, 8:20 a.m. UTC 
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.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 hw/intc/riscv_aplic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Henrique Barboza Aug. 8, 2024, 9:40 p.m. UTC | #1 
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")

> 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."


Thanks,

Daniel


>               return;
>           }
>           if ((aplic->state[irq] & APLIC_ISTATE_INPUT) &&
Yong-Xuan Wang Aug. 9, 2024, 3:28 a.m. UTC | #2 
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) &&
diff mbox series

Patch

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) {
             return;
         }
         if ((aplic->state[irq] & APLIC_ISTATE_INPUT) &&