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

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

[Yong-Xuan Wang]

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(-)

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) &&
-- 
2.17.1

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

[Daniel Henrique Barboza]

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) &&

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) &&

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

[Yong-Xuan Wang]

ping

On Fri, Aug 9, 2024 at 11:28 AM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>
> 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) &&

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

[Anup Patel]

On Thu, Aug 8, 2024 at 1:51 PM Yong-Xuan Wang <yongxuan.wang@sifive.com> 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.

Same comments at the kernel patch.
https://lore.kernel.org/kvm/CAAhSdy3NmwbHY9Qef9LUeXfr0iE7wC-u0d_fHzC47PXk-MzmRg@mail.gmail.com/

Regards,
Anup

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