| Message ID | 20211124182246.67691-1-shashi.mallela@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc: cannot clear GICv3 ITS CTLR[Enabled] bit | expand |
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.
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
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
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
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:
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(-)