* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.