[PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[Shashi Mallela]

When Enabled bit is cleared in GITS_CTLR,ITS feature continues
to be enabled.This patch fixes the issue.

Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
 hw/intc/arm_gicv3_its.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 84bcbb5f56..c929a9cb5c 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -896,13 +896,14 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
 
     switch (offset) {
     case GITS_CTLR:
-        s->ctlr |= (value & ~(s->ctlr));
-
-        if (s->ctlr & ITS_CTLR_ENABLED) {
+        if (value & R_GITS_CTLR_ENABLED_MASK) {
+            s->ctlr |= ITS_CTLR_ENABLED;
             extract_table_params(s);
             extract_cmdq_params(s);
             s->creadr = 0;
             process_cmdq(s);
+        } else {
+            s->ctlr &= ~ITS_CTLR_ENABLED;
         }
         break;
     case GITS_CBASER:
-- 
2.27.0

Re: [PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[Alex Bennée]

Shashi Mallela <shashi.mallela@linaro.org> writes:

> When Enabled bit is cleared in GITS_CTLR,ITS feature continues
> to be enabled.This patch fixes the issue.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>


Tested-by: Alex Bennée <alex.bennee@linaro.org>

in so far as it doesn't break the kvm-unit-tests but it also doesn't
solve the:

  irq 55: nobody cared (try booting with the "irqpoll" option)
  CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.15.1-ajb #67
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   dump_backtrace+0x0/0x1ac
   show_stack+0x18/0x24
   dump_stack_lvl+0x68/0x84
   dump_stack+0x18/0x34
   __report_bad_irq+0x4c/0x168
   note_interrupt+0x278/0x420
   handle_irq_event+0x84/0x1a0
   handle_fasteoi_irq+0x148/0x214
   handle_domain_irq+0x60/0x90
   gic_handle_irq+0xb0/0x120
   call_on_irq_stack+0x2c/0x5c
   do_interrupt_handler+0x40/0x58
   el1_interrupt+0x30/0x50
   el1h_64_irq_handler+0x18/0x24
   el1h_64_irq+0x78/0x7c
   finish_task_switch.isra.0+0x174/0x290
   __schedule+0x5e0/0x674
   __cond_resched+0x24/0x50
   run_ksoftirqd+0x44/0x5c
   smpboot_thread_fn+0x154/0x180
   kthread+0x118/0x130
   ret_from_fork+0x10/0x20
  handlers:
  [<0000000050cdc74a>] vring_interrupt
  Disabling IRQ #55

that is being seen on newer kernels.

-- 
Alex Bennée

Re: [PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[Peter Maydell]

On Wed, 24 Nov 2021 at 18:22, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> When Enabled bit is cleared in GITS_CTLR,ITS feature continues
> to be enabled.This patch fixes the issue.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
>  hw/intc/arm_gicv3_its.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 84bcbb5f56..c929a9cb5c 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -896,13 +896,14 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
>
>      switch (offset) {
>      case GITS_CTLR:
> -        s->ctlr |= (value & ~(s->ctlr));
> -
> -        if (s->ctlr & ITS_CTLR_ENABLED) {
> +        if (value & R_GITS_CTLR_ENABLED_MASK) {
> +            s->ctlr |= ITS_CTLR_ENABLED;
>              extract_table_params(s);
>              extract_cmdq_params(s);
>              s->creadr = 0;
>              process_cmdq(s);
> +        } else {
> +            s->ctlr &= ~ITS_CTLR_ENABLED;
>          }
>          break;
>      case GITS_CBASER:

The code looks fine, so in that sense
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

It seems odd that we have two different #defines for the
same bit, though (ITS_CTLR_ENABLED and R_GITS_CTLR_ENABLED_MASK).
We should probably standardize on the latter and drop the
former.

thanks
-- PMM

Re: [PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[Peter Maydell]

On Thu, 25 Nov 2021 at 15:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Shashi Mallela <shashi.mallela@linaro.org> writes:
>
> > When Enabled bit is cleared in GITS_CTLR,ITS feature continues
> > to be enabled.This patch fixes the issue.
> >
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
>
>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>
> in so far as it doesn't break the kvm-unit-tests but it also doesn't
> solve the:
>
>   irq 55: nobody cared (try booting with the "irqpoll" option)

For the fix to that try
https://patchew.org/QEMU/20211124202005.989935-1-peter.maydell@linaro.org/

-- PMM

Re: [PATCH] hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit

[Peter Maydell]

On Thu, 25 Nov 2021 at 15:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 24 Nov 2021 at 18:22, Shashi Mallela <shashi.mallela@linaro.org> wrote:
> >
> > When Enabled bit is cleared in GITS_CTLR,ITS feature continues
> > to be enabled.This patch fixes the issue.
> >
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > ---
> >  hw/intc/arm_gicv3_its.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 84bcbb5f56..c929a9cb5c 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -896,13 +896,14 @@ static bool its_writel(GICv3ITSState *s, hwaddr offset,
> >
> >      switch (offset) {
> >      case GITS_CTLR:
> > -        s->ctlr |= (value & ~(s->ctlr));
> > -
> > -        if (s->ctlr & ITS_CTLR_ENABLED) {
> > +        if (value & R_GITS_CTLR_ENABLED_MASK) {
> > +            s->ctlr |= ITS_CTLR_ENABLED;
> >              extract_table_params(s);
> >              extract_cmdq_params(s);
> >              s->creadr = 0;
> >              process_cmdq(s);
> > +        } else {
> > +            s->ctlr &= ~ITS_CTLR_ENABLED;
> >          }
> >          break;
> >      case GITS_CBASER:
>
> The code looks fine, so in that sense
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> It seems odd that we have two different #defines for the
> same bit, though (ITS_CTLR_ENABLED and R_GITS_CTLR_ENABLED_MASK).
> We should probably standardize on the latter and drop the
> former.

Applied this version to target-arm.next for 6.2, anyway.

-- PMM