Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-03-07 17:58+0100, Andrew Jones:
> On Mon, Mar 06, 2017 at 07:16:09AM -0800, Christoffer Dall wrote:
> > From: Christoffer Dall <cdall@linaro.org>
> > 
> > We found a deadlock when changing the active state of an interrupt while
> > the interrupt is queued on the LR of the running VCPU.
> > 
> > Defend KVM against this bug in the future now when we've introduced a
> > fix.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> > Sending with the right subject prefix this time.
> > 
> >  arm/gic.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  arm/unittests.cfg | 14 +++++++++++++-
> >  lib/arm/asm/gic.h |  2 ++
> >  3 files changed, 58 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arm/gic.c b/arm/gic.c
> > index 3054d45..82f6632 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -254,6 +254,47 @@ static struct gic gicv3 = {
> >  	},
> >  };
> >  
> > +static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> > +{
> > +	u32 irqstat = gic_read_iar();
> > +	u32 irqnr = gic_iar_irqnr(irqstat);
> > +
> > +	if (irqnr != GICC_INT_SPURIOUS) {
> > +		void *base;
> > +		u32 val = 1 << IPI_IRQ;
> > +
> > +		if (gic_version() == 2)
> > +			base = gicv2_dist_base();
> > +		else
> > +			base = gicv3_redist_base();
> 
> Using the redistributor interface is correct with the current
> gic_enable_defaults(), because it enables affinity routing. I
> wonder if we shouldn't confirm it's enabled first though.
> 
> > +
> > +		writel(val, base + GICD_ICACTIVER);
> 
> For gicv3, with affinity routing enabled, the offset is
> technically named GICR_ICACTIVER0, but that's also 0x380, so
> it doesn't matter.
> 
> Not sure we need the 'val' variable.
> 
> > +
> > +		smp_rmb(); /* pairs with wmb in stats_reset */
> > +		++acked[smp_processor_id()];
> > +		check_irqnr(irqnr);
> > +		smp_wmb(); /* pairs with rmb in check_acked */
> > +	} else {
> > +		++spurious[smp_processor_id()];
> > +		smp_wmb();
> > +	}
> > +}
> > +
> > +static void run_active_clear_test(void)
> > +{
> > +	report_prefix_push("active");
> > +	gic_enable_defaults();
> > +#ifdef __arm__
> > +	install_exception_handler(EXCPTN_IRQ, ipi_clear_active_handler);
> > +#else
> > +	install_irq_handler(EL1H_IRQ, ipi_clear_active_handler);
> > +#endif
> > +	local_irq_enable();
> > +
> > +	ipi_test_self();
> > +	report_prefix_pop();
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	char pfx[8];
> > @@ -290,6 +331,8 @@ int main(int argc, char **argv)
> >  				cpu == IPI_SENDER ? ipi_send : ipi_recv);
> >  		}
> >  		ipi_recv();
> > +	} else if (strcmp(argv[1], "active") == 0) {
> > +		run_active_clear_test();
> 
> test_active_clear() ?
> 
> >  	} else {
> >  		report_abort("Unknown subtest '%s'", argv[1]);
> >  	}
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index c98658f..32d9858 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -26,7 +26,7 @@
> >  ##############################################################################
> >  
> >  #
> > -# Test that the configured number of processors (smp = <num>), and
> > +/# Test that the configured number of processors (smp = <num>), and
> 
> stray character here
> 
> >  # that the configured amount of memory (-m <MB>) are correctly setup
> >  # by the framework.
> >  #
> > @@ -92,6 +92,18 @@ smp = $MAX_SMP
> >  extra_params = -machine gic-version=3 -append 'ipi'
> >  groups = gic
> >  
> > +[gicv2-active]
> > +file = gic.flat
> > +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> > +extra_params = -machine gic-version=2 -append 'active'
> > +groups = gic
> > +
> > +[gicv3-active]
> > +file = gic.flat
> > +smp = $MAX_SMP
> > +extra_params = -machine gic-version=3 -append 'active'
> > +groups = gic
> > +
> >  # Test PSCI emulation
> >  [psci]
> >  file = psci.flat
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index c8186f2..c688ccc 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -12,6 +12,8 @@
> >  #define GICD_TYPER			0x0004
> >  #define GICD_IGROUPR			0x0080
> >  #define GICD_ISENABLER			0x0100
> > +#define GICD_ISACTIVER			0x0300
> > +#define GICD_ICACTIVER			0x0380
> >  #define GICD_IPRIORITYR			0x0400
> >  #define GICD_SGIR			0x0f00
> >  
> > -- 
> > 2.5.0
> > 
> 
> Everything besides the stray character in unittests.cfg is on
> nit level, and the stray character can probably be cleaned up
> on commit, so
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Christoffer, I have fixed the slash in a local repo, ready to push if
you do not want any other changes.

Btw. can the host be rebooted without magic-SysRq after hitting the bug?
(I was wondering if the bug was bad enough for the nodefault group, but
 Drew didn't point it out and the host should still be useable
 afterwards ...)

Thanks.