* [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts @ 2017-03-06 15:16 Christoffer Dall 2017-03-07 16:58 ` Andrew Jones 0 siblings, 1 reply; 5+ messages in thread From: Christoffer Dall @ 2017-03-06 15:16 UTC (permalink / raw) To: Andrew Jones, kvmarm, kvm, Paolo Bonzini, Radim Krčmář Cc: Marc Zyngier, Christoffer Dall 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(); + + writel(val, base + GICD_ICACTIVER); + + 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(); } 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 # 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts 2017-03-06 15:16 [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts Christoffer Dall @ 2017-03-07 16:58 ` Andrew Jones 2017-03-07 19:34 ` Radim Krčmář 0 siblings, 1 reply; 5+ messages in thread From: Andrew Jones @ 2017-03-07 16:58 UTC (permalink / raw) To: Christoffer Dall Cc: Christoffer Dall, kvm, Marc Zyngier, Paolo Bonzini, kvmarm 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> Thanks, drew ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts 2017-03-07 16:58 ` Andrew Jones @ 2017-03-07 19:34 ` Radim Krčmář 2017-03-08 11:45 ` Christoffer Dall 0 siblings, 1 reply; 5+ messages in thread From: Radim Krčmář @ 2017-03-07 19:34 UTC (permalink / raw) To: Christoffer Dall Cc: Christoffer Dall, kvm, Marc Zyngier, Paolo Bonzini, kvmarm 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts 2017-03-07 19:34 ` Radim Krčmář @ 2017-03-08 11:45 ` Christoffer Dall 2017-03-08 15:40 ` Radim Krčmář 0 siblings, 1 reply; 5+ messages in thread From: Christoffer Dall @ 2017-03-08 11:45 UTC (permalink / raw) To: Radim Krčmář Cc: Christoffer Dall, kvm, Marc Zyngier, Paolo Bonzini, kvmarm On Tue, Mar 07, 2017 at 08:34:33PM +0100, Radim Krčmář wrote: > 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 ...) > Good question. I just tested this with the broken kernel, and while you do have a thread that doesn't exit and spins a bit, then your host is completely usable and can be normally rebooted, so I think this is actually ok to leave as a default, but it's up to you. Thanks, -Christoffer _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts 2017-03-08 11:45 ` Christoffer Dall @ 2017-03-08 15:40 ` Radim Krčmář 0 siblings, 0 replies; 5+ messages in thread From: Radim Krčmář @ 2017-03-08 15:40 UTC (permalink / raw) To: Christoffer Dall Cc: Christoffer Dall, kvm, Marc Zyngier, Paolo Bonzini, kvmarm 2017-03-08 03:45-0800, Christoffer Dall: > On Tue, Mar 07, 2017 at 08:34:33PM +0100, Radim Krčmář wrote: > > 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 ...) > > > > Good question. I just tested this with the broken kernel, and while you > do have a thread that doesn't exit and spins a bit, then your host is > completely usable and can be normally rebooted, so I think this is > actually ok to leave as a default, but it's up to you. I agree, nodefault is for worse bugs. Pushed, thanks. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-08 15:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-06 15:16 [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts Christoffer Dall 2017-03-07 16:58 ` Andrew Jones 2017-03-07 19:34 ` Radim Krčmář 2017-03-08 11:45 ` Christoffer Dall 2017-03-08 15:40 ` Radim Krčmář
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).