[kvm-unit-tests PATCH 00/17] arm: gic: Test SPIs and interrupt groups

[kvm-unit-tests PATCH 00/17] arm: gic: Test SPIs and interrupt groups

[Andre Przywara]

So far the GIC testing was limited: we were only testing some MMIO
properties on a GICv2 and some IPI behaviour.
This series extends this to cover SPIs as well, also testing the
behaviour with the two interrupt groups the emulated GIC provides to us.

The first patch is an easy extension to allow some distributor MMIO
tests on GICv3 guests as well.
Patch 2 - 5 prepare the GIC testing framework to handle SPIs, also to
be able to check for interrupts *not* firing.
This is used in patch 6, which adds an IRQ test by using a software
triggered SPI. This also tests whether interrupt masking works, at least
using one of the many masking methods.
Patch 8 adds a test to cover multiple cores, to test the interrupt
target settings. Patch 7 avoids unneccesary output when triggering one IRQ
multiple times.
The remainder of the patches add code to differentiate the two interrupt
groups that the GIC provides. On bare-metal machines, typically having
some secure world code running, and using the typical dual-security-state
GIC configuration, we only ever have one group (group 1) available, so
Linux won't use this feature.
However our emulated GIC does not have a secure side (as we are running
in at most EL2), so both interrupt groups are available to a guest.
So the patches add support to program interrupts to belong to one or
another group, also program the virtual GIC to deliver group 0
interrupts as FIQs, and group 1 interrupts as IRQs.
We then do some tests to see whether this setup works as expected.

At the moment only QEMU using TCG passes these tests, the KVM VGIC
emulation fails to handle the group enable bits properly. Patches for
the kernel will be send shortly, this series acts as a verification test
for this feature (as normal world OSes won't probably use two groups).

Please have a look and comment. I am not particularly happy with patch
16, thrilled to hear any suggestions on how to handle this better.

Tested on arm/GICv2, arm64/GICv2 and arm64/GICv3, with and without the
corresponding KVM patches to fix the dual-group behaviour.

Cheers,
Andre

Andre Przywara (17):
  arm: gic: Enable GIC MMIO tests for GICv3 as well
  arm: gic: Generalise function names
  arm: gic: Provide per-IRQ helper functions
  arm: gic: Support no IRQs test case
  arm: gic: Prepare IRQ handler for handling SPIs
  arm: gic: Add simple shared IRQ test
  arm: gic: Extend check_acked() to allow silent call
  arm: gic: Add simple SPI MP test
  arm: gic: Add test for flipping GICD_CTLR.DS
  arm: gic: Check for writable IGROUPR registers
  arm: gic: Check for validity of both group enable bits
  arm: gic: Change gic_read_iar() to take group parameter
  arm: gic: Change write_eoir() to take group parameter
  arm: gic: Prepare for receiving GIC group 0 interrupts via FIQs
  arm: gic: Provide FIQ handler
  arm: gic: Prepare interrupt statistics for both groups
  arm: gic: Test Group0 SPIs

 arm/gic.c                  | 468 ++++++++++++++++++++++++++++++++-----
 arm/micro-bench.c          |   6 +-
 arm/pl031.c                |   4 +-
 arm/timer.c                |   4 +-
 arm/unittests.cfg          |  38 ++-
 lib/arm/asm/arch_gicv3.h   |  26 ++-
 lib/arm/asm/gic-v2.h       |  11 +-
 lib/arm/asm/gic-v3.h       |  12 +-
 lib/arm/asm/gic.h          |  13 +-
 lib/arm/asm/processor.h    |  10 +
 lib/arm/gic-v2.c           |  39 +++-
 lib/arm/gic-v3.c           |   2 +-
 lib/arm/gic.c              | 102 +++++++-
 lib/arm64/asm/arch_gicv3.h |  29 ++-
 lib/arm64/asm/processor.h  |  10 +
 lib/arm64/processor.c      |   2 +
 16 files changed, 685 insertions(+), 91 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 01/17] arm: gic: Enable GIC MMIO tests for GICv3 as well

[Andre Przywara]

So far the GIC MMIO tests were only enabled for a GICv2 guest. Modern
machines tend to have a GICv3-only GIC, so can't run those tests.
It turns out that most GIC distributor registers we test in the unit
tests are actually the same in GICv3, so we can just enable those tests
for GICv3 guests as well.
The only exception is the CPU number in the TYPER register, which is
only valid in the GICv2 compat mode (ARE=0), which KVM does not support.
So we protect this test against running on a GICv3 guest.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c            | 13 +++++++++++--
 arm/unittests.cfg    | 26 ++++++++++++++++++++++----
 lib/arm/asm/gic-v3.h |  2 ++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index adb6aa4..04b3337 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -6,6 +6,7 @@
  *   + MMIO access tests
  * GICv3
  *   + test sending/receiving IPIs
+ *   + MMIO access tests
  *
  * Copyright (C) 2016, Red Hat Inc, Andrew Jones <drjones@redhat.com>
  *
@@ -496,7 +497,14 @@ static void gic_test_mmio(void)
 		idreg = gic_dist_base + GICD_ICPIDR2;
 		break;
 	case 0x3:
-		report_abort("GICv3 MMIO tests NYI");
+		/*
+		 * We only test generic registers or those affecting
+		 * SPIs, so don't need to consider the SGI base in
+		 * the redistributor here.
+		 */
+		gic_dist_base = gicv3_dist_base();
+		idreg = gic_dist_base + GICD_PIDR2;
+		break;
 	default:
 		report_abort("GIC version %d not supported", gic_version());
 	}
@@ -505,7 +513,8 @@ static void gic_test_mmio(void)
 	nr_irqs = GICD_TYPER_IRQS(reg);
 	report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
 
-	test_typer_v2(reg);
+	if (gic_version() == 0x2)
+		test_typer_v2(reg);
 
 	report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));
 
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index daeb5a0..12ac142 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -86,28 +86,46 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
 extra_params = -machine gic-version=2 -append 'ipi'
 groups = gic
 
-[gicv2-mmio]
+[gicv3-ipi]
+file = gic.flat
+smp = $MAX_SMP
+extra_params = -machine gic-version=3 -append 'ipi'
+groups = gic
+
+[gicv2-max-mmio]
 file = gic.flat
 smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
 extra_params = -machine gic-version=2 -append 'mmio'
 groups = gic
 
+[gicv3-max-mmio]
+file = gic.flat
+smp = $MAX_SMP
+extra_params = -machine gic-version=3 -append 'mmio'
+groups = gic
+
 [gicv2-mmio-up]
 file = gic.flat
 smp = 1
 extra_params = -machine gic-version=2 -append 'mmio'
 groups = gic
 
+[gicv3-mmio-up]
+file = gic.flat
+smp = 1
+extra_params = -machine gic-version=3 -append 'mmio'
+groups = gic
+
 [gicv2-mmio-3p]
 file = gic.flat
 smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
 extra_params = -machine gic-version=2 -append 'mmio'
 groups = gic
 
-[gicv3-ipi]
+[gicv3-mmio-3p]
 file = gic.flat
-smp = $MAX_SMP
-extra_params = -machine gic-version=3 -append 'ipi'
+smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
+extra_params = -machine gic-version=2 -append 'mmio'
 groups = gic
 
 [gicv2-active]
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 347be2f..ed6a5ad 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -23,6 +23,8 @@
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_PIDR2			0xffe8
+
 /* Re-Distributor registers, offsets from RD_base */
 #define GICR_TYPER			0x0008
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 02/17] arm: gic: Generalise function names

[Andre Przywara]

In preparation for adding functions to test SPI interrupts, generalise
some existing functions dealing with IPIs so far, since most of them
are actually generic for all kind of interrupts.
This also reformats the irq_handler() function, to later expand it
more easily.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 04b3337..a114009 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -135,28 +135,30 @@ static void check_ipi_sender(u32 irqstat)
 	}
 }
 
-static void check_irqnr(u32 irqnr)
+static void check_irqnr(u32 irqnr, int expected)
 {
-	if (irqnr != IPI_IRQ)
+	if (irqnr != expected)
 		bad_irq[smp_processor_id()] = irqnr;
 }
 
-static void ipi_handler(struct pt_regs *regs __unused)
+static void irq_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
-	if (irqnr != GICC_INT_SPURIOUS) {
-		gic_write_eoir(irqstat);
-		smp_rmb(); /* pairs with wmb in stats_reset */
-		++acked[smp_processor_id()];
-		check_ipi_sender(irqstat);
-		check_irqnr(irqnr);
-		smp_wmb(); /* pairs with rmb in check_acked */
-	} else {
+	if (irqnr == GICC_INT_SPURIOUS) {
 		++spurious[smp_processor_id()];
 		smp_wmb();
+		return;
 	}
+
+	gic_write_eoir(irqstat);
+
+	smp_rmb(); /* pairs with wmb in stats_reset */
+	++acked[smp_processor_id()];
+	check_ipi_sender(irqstat);
+	check_irqnr(irqnr, IPI_IRQ);
+	smp_wmb(); /* pairs with rmb in check_acked */
 }
 
 static void gicv2_ipi_send_self(void)
@@ -216,20 +218,20 @@ static void ipi_test_smp(void)
 	report_prefix_pop();
 }
 
-static void ipi_enable(void)
+static void irqs_enable(void)
 {
 	gic_enable_defaults();
 #ifdef __arm__
-	install_exception_handler(EXCPTN_IRQ, ipi_handler);
+	install_exception_handler(EXCPTN_IRQ, irq_handler);
 #else
-	install_irq_handler(EL1H_IRQ, ipi_handler);
+	install_irq_handler(EL1H_IRQ, irq_handler);
 #endif
 	local_irq_enable();
 }
 
 static void ipi_send(void)
 {
-	ipi_enable();
+	irqs_enable();
 	wait_on_ready();
 	ipi_test_self();
 	ipi_test_smp();
@@ -237,9 +239,9 @@ static void ipi_send(void)
 	exit(report_summary());
 }
 
-static void ipi_recv(void)
+static void irq_recv(void)
 {
-	ipi_enable();
+	irqs_enable();
 	cpumask_set_cpu(smp_processor_id(), &ready);
 	while (1)
 		wfi();
@@ -250,7 +252,7 @@ static void ipi_test(void *data __unused)
 	if (smp_processor_id() == IPI_SENDER)
 		ipi_send();
 	else
-		ipi_recv();
+		irq_recv();
 }
 
 static struct gic gicv2 = {
@@ -285,7 +287,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		smp_rmb(); /* pairs with wmb in stats_reset */
 		++acked[smp_processor_id()];
-		check_irqnr(irqnr);
+		check_irqnr(irqnr, IPI_IRQ);
 		smp_wmb(); /* pairs with rmb in check_acked */
 	} else {
 		++spurious[smp_processor_id()];
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions

[Andre Przywara]

A common theme when accessing per-IRQ parameters in the GIC distributor
is to set fields of a certain bit width in a range of MMIO registers.
Examples are the enabled status (one bit per IRQ), the level/edge
configuration (2 bits per IRQ) or the priority (8 bits per IRQ).

Add a generic helper function which is able to mask and set the
respective number of bits, given the IRQ number and the MMIO offset.
Provide wrappers using this function to easily allow configuring an IRQ.

For now assume that private IRQ numbers always refer to the current CPU.
In a GICv2 accessing the "other" private IRQs is not easily doable (the
registers are banked per CPU on the same MMIO address), so we impose the
same limitation on GICv3, even though those registers are not banked
there anymore.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 lib/arm/asm/gic-v3.h |  1 +
 lib/arm/asm/gic.h    |  9 +++++
 lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index ed6a5ad..8cfaed1 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -23,6 +23,7 @@
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_IROUTER			0x6000
 #define GICD_PIDR2			0xffe8
 
 /* Re-Distributor registers, offsets from RD_base */
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 1fc10a0..21cdb58 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -15,6 +15,7 @@
 #define GICD_IIDR			0x0008
 #define GICD_IGROUPR			0x0080
 #define GICD_ISENABLER			0x0100
+#define GICD_ICENABLER			0x0180
 #define GICD_ISPENDR			0x0200
 #define GICD_ICPENDR			0x0280
 #define GICD_ISACTIVER			0x0300
@@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
 extern void gic_ipi_send_single(int irq, int cpu);
 extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
 
+void gic_set_irq_bit(int irq, int offset);
+void gic_enable_irq(int irq);
+void gic_disable_irq(int irq);
+void gic_set_irq_priority(int irq, u8 prio);
+void gic_set_irq_target(int irq, int cpu);
+void gic_set_irq_group(int irq, int group);
+int gic_get_irq_group(int irq);
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMARM_GIC_H_ */
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 9430116..cf4e811 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
 	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
 	gic_common_ops->ipi_send_mask(irq, dest);
 }
+
+enum gic_bit_access {
+	ACCESS_READ,
+	ACCESS_SET,
+	ACCESS_RMW
+};
+
+static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
+			      enum gic_bit_access access)
+{
+	void *base;
+	int split = 32 / bits;
+	int shift = (irq % split) * bits;
+	u32 reg, mask = ((1U << bits) - 1) << shift;
+
+	switch (gic_version()) {
+	case 2:
+		base = gicv2_dist_base();
+		break;
+	case 3:
+		if (irq < 32)
+			base = gicv3_sgi_base();
+		else
+			base = gicv3_dist_base();
+		break;
+	default:
+		return 0;
+	}
+	base += offset + (irq / split) * 4;
+
+	switch (access) {
+	case ACCESS_READ:
+		return (readl(base) & mask) >> shift;
+	case ACCESS_SET:
+		reg = 0;
+		break;
+	case ACCESS_RMW:
+		reg = readl(base) & ~mask;
+		break;
+	}
+
+	writel(reg | ((u32)value << shift), base);
+
+	return 0;
+}
+
+void gic_set_irq_bit(int irq, int offset)
+{
+	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
+}
+
+void gic_enable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ISENABLER);
+}
+
+void gic_disable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ICENABLER);
+}
+
+void gic_set_irq_priority(int irq, u8 prio)
+{
+	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
+}
+
+void gic_set_irq_target(int irq, int cpu)
+{
+	if (irq < 32)
+		return;
+
+	if (gic_version() == 2) {
+		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
+				    ACCESS_RMW);
+
+		return;
+	}
+
+	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
+}
+
+void gic_set_irq_group(int irq, int group)
+{
+	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
+}
+
+int gic_get_irq_group(int irq)
+{
+	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
+}
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case

[Andre Przywara]

For some tests it would be important to check that an IRQ was *not*
triggered, for instance to test certain masking operations.

Extend the check_added() function to recognise an empty cpumask to
detect this situation. The timeout duration is reduced, and the "no IRQs
triggered" case is actually reported as a success in this case.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a114009..eca9188 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask)
 	int missing = 0, extra = 0, unexpected = 0;
 	int nr_pass, cpu, i;
 	bool bad = false;
+	bool noirqs = cpumask_empty(mask);
 
 	/* Wait up to 5s for all interrupts to be delivered */
-	for (i = 0; i < 50; ++i) {
+	for (i = 0; i < (noirqs ? 15 : 50); ++i) {
 		mdelay(100);
 		nr_pass = 0;
 		for_each_present_cpu(cpu) {
@@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask)
 				bad = true;
 			}
 		}
-		if (nr_pass == nr_cpus) {
+		if (!noirqs && nr_pass == nr_cpus) {
 			report("%s", !bad, testname);
 			if (i)
 				report_info("took more than %d ms", i * 100);
@@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
 		}
 	}
 
+	if (noirqs && nr_pass == nr_cpus) {
+		report("%s", !bad, testname);
+		return;
+	}
+
 	for_each_present_cpu(cpu) {
 		if (cpumask_test_cpu(cpu, mask)) {
 			if (!acked[cpu])
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 05/17] arm: gic: Prepare IRQ handler for handling SPIs

[Andre Przywara]

So far our IRQ handler routine checks that the received IRQ is actually
the one SGI (IPI) that we are using for our testing.

To make the IRQ testing routine more versatile, also allow the IRQ to be
one test SPI (shared interrupt).
We use the penultimate IRQ of the first SPI group for that purpose.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index eca9188..c909668 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -23,6 +23,7 @@
 
 #define IPI_SENDER	1
 #define IPI_IRQ		1
+#define SPI_IRQ		(GIC_FIRST_SPI + 30)
 
 struct gic {
 	struct {
@@ -162,8 +163,12 @@ static void irq_handler(struct pt_regs *regs __unused)
 
 	smp_rmb(); /* pairs with wmb in stats_reset */
 	++acked[smp_processor_id()];
-	check_ipi_sender(irqstat);
-	check_irqnr(irqnr, IPI_IRQ);
+	if (irqnr < GIC_NR_PRIVATE_IRQS) {
+		check_ipi_sender(irqstat);
+		check_irqnr(irqnr, IPI_IRQ);
+	} else {
+		check_irqnr(irqnr, SPI_IRQ);
+	}
 	smp_wmb(); /* pairs with rmb in check_acked */
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 06/17] arm: gic: Add simple shared IRQ test

[Andre Przywara]

So far we were testing the GIC's MMIO interface and IPI delivery.
Add a simple test to trigger a shared IRQ (SPI), using the ISPENDR
register in the (emulated) GIC distributor.
This tests configuration of an IRQ (target CPU) and whether it can be
properly enabled or disabled.

This is a bit more sophisticated than actually needed at this time,
but paves the way for extending this to FIQs in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg | 12 +++++++
 2 files changed, 91 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index c909668..3be76cb 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -546,6 +546,81 @@ static void gic_test_mmio(void)
 		test_targets(nr_irqs);
 }
 
+static void gic_spi_trigger(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ISPENDR);
+}
+
+static void spi_configure_irq(int irq, int cpu)
+{
+	gic_set_irq_target(irq, cpu);
+	gic_set_irq_priority(irq, 0xa0);
+	gic_enable_irq(irq);
+}
+
+#define IRQ_STAT_NONE		0
+#define IRQ_STAT_IRQ		1
+#define IRQ_STAT_TYPE_MASK	0x3
+#define IRQ_STAT_NO_CLEAR	4
+
+/*
+ * Wait for an SPI to fire (or not) on a certain CPU.
+ * Clears the pending bit if requested afterwards.
+ */
+static void trigger_and_check_spi(const char *test_name,
+				  unsigned int irq_stat,
+				  int cpu)
+{
+	cpumask_t cpumask;
+
+	stats_reset();
+	gic_spi_trigger(SPI_IRQ);
+	cpumask_clear(&cpumask);
+	switch (irq_stat & IRQ_STAT_TYPE_MASK) {
+	case IRQ_STAT_NONE:
+		break;
+	case IRQ_STAT_IRQ:
+		cpumask_set_cpu(cpu, &cpumask);
+		break;
+	}
+
+	check_acked(test_name, &cpumask);
+
+	/* Clean up pending bit in case this IRQ wasn't taken. */
+	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
+		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);
+}
+
+static void spi_test_single(void)
+{
+	cpumask_t cpumask;
+	int cpu = smp_processor_id();
+
+	spi_configure_irq(SPI_IRQ, cpu);
+
+	trigger_and_check_spi("SPI triggered by CPU write", IRQ_STAT_IRQ, cpu);
+
+	gic_disable_irq(SPI_IRQ);
+	trigger_and_check_spi("disabled SPI does not fire",
+			      IRQ_STAT_NONE | IRQ_STAT_NO_CLEAR, cpu);
+
+	stats_reset();
+	cpumask_clear(&cpumask);
+	cpumask_set_cpu(cpu, &cpumask);
+	gic_enable_irq(SPI_IRQ);
+	check_acked("now enabled SPI fires", &cpumask);
+}
+
+static void spi_send(void)
+{
+	irqs_enable();
+
+	spi_test_single();
+
+	check_spurious();
+	exit(report_summary());
+}
+
 int main(int argc, char **argv)
 {
 	if (!gic_init()) {
@@ -577,6 +652,10 @@ int main(int argc, char **argv)
 		report_prefix_push(argv[1]);
 		gic_test_mmio();
 		report_prefix_pop();
+	} else if (strcmp(argv[1], "irq") == 0) {
+		report_prefix_push(argv[1]);
+		spi_send();
+		report_prefix_pop();
 	} else {
 		report_abort("Unknown subtest '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 12ac142..7a78275 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -92,6 +92,18 @@ smp = $MAX_SMP
 extra_params = -machine gic-version=3 -append 'ipi'
 groups = gic
 
+[gicv2-spi]
+file = gic.flat
+smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
+extra_params = -machine gic-version=2 -append 'irq'
+groups = gic
+
+[gicv3-spi]
+file = gic.flat
+smp = $MAX_SMP
+extra_params = -machine gic-version=3 -append 'irq'
+groups = gic
+
 [gicv2-max-mmio]
 file = gic.flat
 smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call

[Andre Przywara]

For future tests we will need to call check_acked() twice for the same
interrupt (to test delivery of Group 0 and Group 1 interrupts).
This should be reported as a single test, so allow check_acked() to be
called with a "NULL" test name, to suppress output. We report the test
result via the return value, so the outcome is not lost.

Also this amends the new trigger_and_check_spi() wrapper, to propagate
the test result to callers of that function.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 3be76cb..63aa9f4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -62,7 +62,7 @@ static void stats_reset(void)
 	smp_wmb();
 }
 
-static void check_acked(const char *testname, cpumask_t *mask)
+static int check_acked(const char *testname, cpumask_t *mask)
 {
 	int missing = 0, extra = 0, unexpected = 0;
 	int nr_pass, cpu, i;
@@ -91,16 +91,20 @@ static void check_acked(const char *testname, cpumask_t *mask)
 			}
 		}
 		if (!noirqs && nr_pass == nr_cpus) {
-			report("%s", !bad, testname);
-			if (i)
-				report_info("took more than %d ms", i * 100);
-			return;
+			if (testname) {
+				report("%s", !bad, testname);
+				if (i)
+					report_info("took more than %d ms",
+						    i * 100);
+			}
+			return i * 100;
 		}
 	}
 
 	if (noirqs && nr_pass == nr_cpus) {
-		report("%s", !bad, testname);
-		return;
+		if (testname)
+			report("%s", !bad, testname);
+		return i * 100;
 	}
 
 	for_each_present_cpu(cpu) {
@@ -115,9 +119,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
 		}
 	}
 
-	report("%s", false, testname);
+	if (testname)
+		report("%s", false, testname);
 	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
 		    missing, extra, unexpected);
+	return -1;
 }
 
 static void check_spurious(void)
@@ -567,11 +573,12 @@ static void spi_configure_irq(int irq, int cpu)
  * Wait for an SPI to fire (or not) on a certain CPU.
  * Clears the pending bit if requested afterwards.
  */
-static void trigger_and_check_spi(const char *test_name,
+static bool trigger_and_check_spi(const char *test_name,
 				  unsigned int irq_stat,
 				  int cpu)
 {
 	cpumask_t cpumask;
+	bool ret = true;
 
 	stats_reset();
 	gic_spi_trigger(SPI_IRQ);
@@ -584,11 +591,13 @@ static void trigger_and_check_spi(const char *test_name,
 		break;
 	}
 
-	check_acked(test_name, &cpumask);
+	ret = (check_acked(test_name, &cpumask) >= 0);
 
 	/* Clean up pending bit in case this IRQ wasn't taken. */
 	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
 		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);
+
+	return ret;
 }
 
 static void spi_test_single(void)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 08/17] arm: gic: Add simple SPI MP test

[Andre Przywara]

Shared Peripheral Interrupts (SPI) can target a specific CPU. Test this
feature by routing the test SPI to each of the vCPUs, then triggering it
and confirm its reception on that requested core.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/arm/gic.c b/arm/gic.c
index 63aa9f4..304b7b9 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -620,16 +620,45 @@ static void spi_test_single(void)
 	check_acked("now enabled SPI fires", &cpumask);
 }
 
+static void spi_test_smp(void)
+{
+	int cpu;
+	int cores = 1;
+
+	wait_on_ready();
+	for_each_present_cpu(cpu) {
+		if (cpu == smp_processor_id())
+			continue;
+		spi_configure_irq(SPI_IRQ, cpu);
+		if (trigger_and_check_spi(NULL, IRQ_STAT_IRQ, cpu))
+			cores++;
+		else
+			report_info("SPI delivery failed on core %d", cpu);
+	}
+	report("SPI delievered on all cores", cores == nr_cpus);
+}
+
 static void spi_send(void)
 {
 	irqs_enable();
 
 	spi_test_single();
 
+	if (nr_cpus > 1)
+		spi_test_smp();
+
 	check_spurious();
 	exit(report_summary());
 }
 
+static void spi_test(void *data __unused)
+{
+	if (smp_processor_id() == 0)
+		spi_send();
+	else
+		irq_recv();
+}
+
 int main(int argc, char **argv)
 {
 	if (!gic_init()) {
@@ -663,7 +692,7 @@ int main(int argc, char **argv)
 		report_prefix_pop();
 	} else if (strcmp(argv[1], "irq") == 0) {
 		report_prefix_push(argv[1]);
-		spi_send();
+		on_cpus(spi_test, NULL);
 		report_prefix_pop();
 	} else {
 		report_abort("Unknown subtest '%s'", argv[1]);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Andre Przywara]

The DS (Disable Security) bit in the GICv3 GICD_CTLR register controls
access to Group 0 interrupts from the non-secure side.
The KVM VGIC emulation provides a "GIC with a single security state",
so both groups should be accessible.
Provide a test to check this bit can be set to one. The current KVM
emulation should treat this is as RAO/WI (which we also check here). It
would be architecturally compliant though to have this bit at 0 as well,
so we refrain from treating different behaviour as a FAIL.
However we use this as a gateway for further Group 0 IRQ tests.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c            | 62 ++++++++++++++++++++++++++++++++++++++++++++
 lib/arm/asm/gic-v3.h |  1 +
 2 files changed, 63 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index 304b7b9..c882a24 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -531,6 +531,8 @@ static void gic_test_mmio(void)
 	reg = readl(gic_dist_base + GICD_TYPER);
 	nr_irqs = GICD_TYPER_IRQS(reg);
 	report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
+	report_info("GIC %s security extension",
+		reg & (1U << 10) ? "has" : "does not have");
 
 	if (gic_version() == 0x2)
 		test_typer_v2(reg);
@@ -638,6 +640,60 @@ static void spi_test_smp(void)
 	report("SPI delievered on all cores", cores == nr_cpus);
 }
 
+/*
+ * Check the security state configuration of the GIC.
+ * Test whether we can switch to a single security state, to test both
+ * group 0 and group 1 interrupts.
+ * Architecturally a GIC can be configured in different ways, so we don't
+ * insist on the current way KVM emulates the GIC.
+ */
+static bool gicv3_check_security(void *gicd_base)
+{
+	u32 ctlr = readl(gicd_base + GICD_CTLR);
+
+	if (ctlr & GICD_CTLR_DS) {
+		writel(ctlr & ~GICD_CTLR_DS, gicd_base + GICD_CTLR);
+		ctlr = readl(gicd_base + GICD_CTLR);
+		if (!(ctlr & GICD_CTLR_DS))
+			report_info("GIC allowing two security states");
+		else
+			report_info("GIC is one security state only");
+	} else {
+		report_info("GIC resets to two security states");
+	}
+
+	writel(ctlr | GICD_CTLR_DS, gicd_base + GICD_CTLR);
+	ctlr = readl(gicd_base + GICD_CTLR);
+	report("switching to single security state", ctlr & GICD_CTLR_DS);
+
+	/* Group0 delivery only works in single security state. */
+	return ctlr & GICD_CTLR_DS;
+}
+
+/*
+ * The GIC architecture describes two interrupt groups, group 0 and group 1.
+ * On bare-metal systems, running in non-secure world on a GIC with the
+ * security extensions, there is only one group available: group 1.
+ * However in the kernel KVM emulates a GIC with only one security state,
+ * so both groups are available to guests.
+ * Check whether this works as expected (as Linux will not use this feature).
+ * We can only verify this state on a GICv3, so we check it there and silently
+ * assume it's valid for GICv2.
+ */
+static void test_irq_group(void *gicd_base)
+{
+	bool is_gicv3 = (gic_version() == 3);
+
+	report_prefix_push("GROUP");
+	gic_enable_defaults();
+
+	if (is_gicv3) {
+		/* GICv3 features a bit to read and set the security state. */
+		if (!gicv3_check_security(gicd_base))
+			return;
+	}
+}
+
 static void spi_send(void)
 {
 	irqs_enable();
@@ -647,6 +703,12 @@ static void spi_send(void)
 	if (nr_cpus > 1)
 		spi_test_smp();
 
+	if (gic_version() == 3)
+		test_irq_group(gicv3_dist_base());
+
+	if (gic_version() == 2)
+		test_irq_group(gicv2_dist_base());
+
 	check_spurious();
 	exit(report_summary());
 }
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 8cfaed1..2eaf944 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -19,6 +19,7 @@
  * group1 enable bits with respect to that view.
  */
 #define GICD_CTLR_RWP			(1U << 31)
+#define GICD_CTLR_DS			(1U << 6)
 #define GICD_CTLR_ARE_NS		(1U << 4)
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 10/17] arm: gic: Check for writable IGROUPR registers

[Andre Przywara]

When both groups are avaiable to the non-secure side, the MMIO group
registers need to be writable, so that the group that an IRQ belongs to
can be programmed.

Check that the group can be flipped, after having established that both
groups are usable.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index c882a24..485ca4f 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -683,6 +683,7 @@ static bool gicv3_check_security(void *gicd_base)
 static void test_irq_group(void *gicd_base)
 {
 	bool is_gicv3 = (gic_version() == 3);
+	u32 reg;
 
 	report_prefix_push("GROUP");
 	gic_enable_defaults();
@@ -692,6 +693,16 @@ static void test_irq_group(void *gicd_base)
 		if (!gicv3_check_security(gicd_base))
 			return;
 	}
+
+	/*
+	 * On a security aware GIC in non-secure world the IGROUPR registers
+	 * are RAZ/WI. KVM emulates a single-security-state GIC, so both
+	 * groups are available and the IGROUPR registers are writable.
+	 */
+	reg = gic_get_irq_group(SPI_IRQ);
+	gic_set_irq_group(SPI_IRQ, !reg);
+	report("IGROUPR is writable", gic_get_irq_group(SPI_IRQ) != reg);
+	gic_set_irq_group(SPI_IRQ, reg);
 }
 
 static void spi_send(void)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 11/17] arm: gic: Check for validity of both group enable bits

[Andre Przywara]

The GIC distributor actually supports *two* enable bits, one per
interrupt group. Linux itself won't care and will only ever use one bit.
In a VM however we have two groups available, so we should be able to
flip the two separate enable bits.

Provide tests that try to flip the two available bits and check whether
they stick.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c            | 21 +++++++++++++++++++++
 lib/arm/asm/gic-v3.h |  4 ++--
 lib/arm/gic-v3.c     |  2 +-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 485ca4f..a0511e5 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -640,6 +640,8 @@ static void spi_test_smp(void)
 	report("SPI delievered on all cores", cores == nr_cpus);
 }
 
+#define GICD_CTLR_ENABLE_BOTH (GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1)
+
 /*
  * Check the security state configuration of the GIC.
  * Test whether we can switch to a single security state, to test both
@@ -694,6 +696,25 @@ static void test_irq_group(void *gicd_base)
 			return;
 	}
 
+	/* Check whether the group enable bits stick. */
+	reg = readl(gicd_base + GICD_CTLR);
+	writel(reg & ~GICD_CTLR_ENABLE_BOTH, gicd_base + GICD_CTLR);
+	reg = readl(gicd_base + GICD_CTLR);
+	report("both groups disabled sticks",
+	       (reg & GICD_CTLR_ENABLE_BOTH) == 0);
+
+	reg &= ~GICD_CTLR_ENABLE_BOTH;
+	writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
+	reg = readl(gicd_base + GICD_CTLR);
+	report("group 1 enabled sticks",
+	       (reg & GICD_CTLR_ENABLE_BOTH) == GICD_CTLR_ENABLE_G1);
+
+	reg &= ~GICD_CTLR_ENABLE_BOTH;
+	writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
+	reg = readl(gicd_base + GICD_CTLR);
+	report("group 0 enabled sticks",
+	       (reg & GICD_CTLR_ENABLE_BOTH) == GICD_CTLR_ENABLE_G0);
+
 	/*
 	 * On a security aware GIC in non-secure world the IGROUPR registers
 	 * are RAZ/WI. KVM emulates a single-security-state GIC, so both
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 2eaf944..0a29610 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -21,8 +21,8 @@
 #define GICD_CTLR_RWP			(1U << 31)
 #define GICD_CTLR_DS			(1U << 6)
 #define GICD_CTLR_ARE_NS		(1U << 4)
-#define GICD_CTLR_ENABLE_G1A		(1U << 1)
-#define GICD_CTLR_ENABLE_G1		(1U << 0)
+#define GICD_CTLR_ENABLE_G1		(1U << 1)
+#define GICD_CTLR_ENABLE_G0		(1U << 0)
 
 #define GICD_IROUTER			0x6000
 #define GICD_PIDR2			0xffe8
diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
index feecb5e..d6a5186 100644
--- a/lib/arm/gic-v3.c
+++ b/lib/arm/gic-v3.c
@@ -42,7 +42,7 @@ void gicv3_enable_defaults(void)
 	writel(0, dist + GICD_CTLR);
 	gicv3_dist_wait_for_rwp();
 
-	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1,
+	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1,
 	       dist + GICD_CTLR);
 	gicv3_dist_wait_for_rwp();
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 12/17] arm: gic: Change gic_read_iar() to take group parameter

[Andre Przywara]

Acknowledging a GIC group 0 interrupt requires us to use a different
system register on GICv3. To allow us to differentiate the two groups
later, add a group parameter to gic_read_iar(). For GICv2 we can use the
same CPU interface register to acknowledge group 0 as well, so we ignore
the parameter here.

For now this is still using group 1 on every caller.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                  |  4 ++--
 arm/micro-bench.c          |  2 +-
 arm/pl031.c                |  2 +-
 arm/timer.c                |  2 +-
 lib/arm/asm/arch_gicv3.h   | 11 +++++++++--
 lib/arm/asm/gic-v2.h       |  2 +-
 lib/arm/asm/gic-v3.h       |  2 +-
 lib/arm/asm/gic.h          |  2 +-
 lib/arm/gic-v2.c           |  3 ++-
 lib/arm/gic.c              |  6 +++---
 lib/arm64/asm/arch_gicv3.h | 10 ++++++++--
 11 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a0511e5..7be13a6 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -156,7 +156,7 @@ static void check_irqnr(u32 irqnr, int expected)
 
 static void irq_handler(struct pt_regs *regs __unused)
 {
-	u32 irqstat = gic_read_iar();
+	u32 irqstat = gic_read_iar(1);
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr == GICC_INT_SPURIOUS) {
@@ -288,7 +288,7 @@ static struct gic gicv3 = {
 
 static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 {
-	u32 irqstat = gic_read_iar();
+	u32 irqstat = gic_read_iar(1);
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr != GICC_INT_SPURIOUS) {
diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index 4612f41..2bfee68 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -33,7 +33,7 @@ static void ipi_irq_handler(struct pt_regs *regs)
 {
 	ipi_ready = false;
 	ipi_received = true;
-	gic_write_eoir(gic_read_iar());
+	gic_write_eoir(gic_read_iar(1));
 	ipi_ready = true;
 }
 
diff --git a/arm/pl031.c b/arm/pl031.c
index 5672f36..5be3d76 100644
--- a/arm/pl031.c
+++ b/arm/pl031.c
@@ -134,7 +134,7 @@ static void gic_irq_unmask(void)
 
 static void irq_handler(struct pt_regs *regs)
 {
-	u32 irqstat = gic_read_iar();
+	u32 irqstat = gic_read_iar(1);
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	gic_write_eoir(irqstat);
diff --git a/arm/timer.c b/arm/timer.c
index 0b808d5..e5cc3b4 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -150,7 +150,7 @@ static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
 static void irq_handler(struct pt_regs *regs)
 {
 	struct timer_info *info;
-	u32 irqstat = gic_read_iar();
+	u32 irqstat = gic_read_iar(1);
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr != GICC_INT_SPURIOUS)
diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
index 45b6096..52e7bba 100644
--- a/lib/arm/asm/arch_gicv3.h
+++ b/lib/arm/asm/arch_gicv3.h
@@ -16,6 +16,7 @@
 
 #define ICC_PMR				__ACCESS_CP15(c4, 0, c6, 0)
 #define ICC_SGI1R			__ACCESS_CP15_64(0, c12)
+#define ICC_IAR0			__ACCESS_CP15(c12, 0,  c8, 0)
 #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
 #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
 #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
@@ -30,9 +31,15 @@ static inline void gicv3_write_sgi1r(u64 val)
 	write_sysreg(val, ICC_SGI1R);
 }
 
-static inline u32 gicv3_read_iar(void)
+static inline u32 gicv3_read_iar(int group)
 {
-	u32 irqstat = read_sysreg(ICC_IAR1);
+	u32 irqstat;
+
+	if (group == 0)
+		irqstat = read_sysreg(ICC_IAR0);
+	else
+		irqstat = read_sysreg(ICC_IAR1);
+
 	dsb(sy);
 	return irqstat;
 }
diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
index 1fcfd43..d50c610 100644
--- a/lib/arm/asm/gic-v2.h
+++ b/lib/arm/asm/gic-v2.h
@@ -32,7 +32,7 @@ extern struct gicv2_data gicv2_data;
 
 extern int gicv2_init(void);
 extern void gicv2_enable_defaults(void);
-extern u32 gicv2_read_iar(void);
+extern u32 gicv2_read_iar(int group);
 extern u32 gicv2_iar_irqnr(u32 iar);
 extern void gicv2_write_eoir(u32 irqstat);
 extern void gicv2_ipi_send_single(int irq, int cpu);
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 0a29610..ca19110 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -69,7 +69,7 @@ extern struct gicv3_data gicv3_data;
 
 extern int gicv3_init(void);
 extern void gicv3_enable_defaults(void);
-extern u32 gicv3_read_iar(void);
+extern u32 gicv3_read_iar(int group);
 extern u32 gicv3_iar_irqnr(u32 iar);
 extern void gicv3_write_eoir(u32 irqstat);
 extern void gicv3_ipi_send_single(int irq, int cpu);
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 21cdb58..09663e7 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -68,7 +68,7 @@ extern void gic_enable_defaults(void);
  * below will work with any supported gic version.
  */
 extern int gic_version(void);
-extern u32 gic_read_iar(void);
+extern u32 gic_read_iar(int group);
 extern u32 gic_iar_irqnr(u32 iar);
 extern void gic_write_eoir(u32 irqstat);
 extern void gic_ipi_send_single(int irq, int cpu);
diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
index dc6a97c..b60967e 100644
--- a/lib/arm/gic-v2.c
+++ b/lib/arm/gic-v2.c
@@ -26,8 +26,9 @@ void gicv2_enable_defaults(void)
 	writel(GICC_ENABLE, cpu_base + GICC_CTLR);
 }
 
-u32 gicv2_read_iar(void)
+u32 gicv2_read_iar(int group)
 {
+	/* GICv2 acks both group0 and group1 IRQs with the same register. */
 	return readl(gicv2_cpu_base() + GICC_IAR);
 }
 
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index cf4e811..b51eff5 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -12,7 +12,7 @@ struct gicv3_data gicv3_data;
 
 struct gic_common_ops {
 	void (*enable_defaults)(void);
-	u32 (*read_iar)(void);
+	u32 (*read_iar)(int group);
 	u32 (*iar_irqnr)(u32 iar);
 	void (*write_eoir)(u32 irqstat);
 	void (*ipi_send_single)(int irq, int cpu);
@@ -117,10 +117,10 @@ void gic_enable_defaults(void)
 	gic_common_ops->enable_defaults();
 }
 
-u32 gic_read_iar(void)
+u32 gic_read_iar(int group)
 {
 	assert(gic_common_ops && gic_common_ops->read_iar);
-	return gic_common_ops->read_iar();
+	return gic_common_ops->read_iar(group);
 }
 
 u32 gic_iar_irqnr(u32 iar)
diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
index a7994ec..876e1fc 100644
--- a/lib/arm64/asm/arch_gicv3.h
+++ b/lib/arm64/asm/arch_gicv3.h
@@ -11,6 +11,7 @@
 #include <asm/sysreg.h>
 
 #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
+#define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
 #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
 #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
 #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
@@ -38,10 +39,15 @@ static inline void gicv3_write_sgi1r(u64 val)
 	asm volatile("msr_s " xstr(ICC_SGI1R_EL1) ", %0" : : "r" (val));
 }
 
-static inline u32 gicv3_read_iar(void)
+static inline u32 gicv3_read_iar(int group)
 {
 	u64 irqstat;
-	asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
+
+	if (group == 0)
+		asm volatile("mrs_s %0, " xstr(ICC_IAR0_EL1) : "=r" (irqstat));
+	else
+		asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
+
 	dsb(sy);
 	return (u64)irqstat;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 13/17] arm: gic: Change write_eoir() to take group parameter

[Andre Przywara]

EOIing GIC group 0 interrupt requires us to use a different
system register on GICv3. To allow us to differentiate the two groups
later, add a group parameter to write_eoir(). For GICv2 we can use the
same CPU interface register to EOI group 0 IRQs as well, so we ignore
the parameter here.

For now this is still using group 1 on every caller.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                  |  2 +-
 arm/micro-bench.c          |  6 +++---
 arm/pl031.c                |  2 +-
 arm/timer.c                |  2 +-
 lib/arm/asm/arch_gicv3.h   |  8 ++++++--
 lib/arm/asm/gic-v2.h       |  2 +-
 lib/arm/asm/gic-v3.h       |  2 +-
 lib/arm/asm/gic.h          |  2 +-
 lib/arm/gic-v2.c           |  2 +-
 lib/arm/gic.c              |  6 +++---
 lib/arm64/asm/arch_gicv3.h | 10 ++++++++--
 11 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 7be13a6..c68b5b5 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -165,7 +165,7 @@ static void irq_handler(struct pt_regs *regs __unused)
 		return;
 	}
 
-	gic_write_eoir(irqstat);
+	gic_write_eoir(irqstat, 1);
 
 	smp_rmb(); /* pairs with wmb in stats_reset */
 	++acked[smp_processor_id()];
diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index 2bfee68..a8f100f 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -27,13 +27,13 @@ static u32 cntfrq;
 
 static volatile bool ipi_ready, ipi_received;
 static void *vgic_dist_base;
-static void (*write_eoir)(u32 irqstat);
+static void (*write_eoir)(u32 irqstat, int group);
 
 static void ipi_irq_handler(struct pt_regs *regs)
 {
 	ipi_ready = false;
 	ipi_received = true;
-	gic_write_eoir(gic_read_iar(1));
+	gic_write_eoir(gic_read_iar(1), 1);
 	ipi_ready = true;
 }
 
@@ -135,7 +135,7 @@ static void eoi_exec(void)
 	int spurious_id = 1023; /* writes to EOI are ignored */
 
 	/* Avoid measuring assert(..) in gic_write_eoir */
-	write_eoir(spurious_id);
+	write_eoir(spurious_id, 1);
 }
 
 struct exit_test {
diff --git a/arm/pl031.c b/arm/pl031.c
index 5be3d76..7103e7b 100644
--- a/arm/pl031.c
+++ b/arm/pl031.c
@@ -137,7 +137,7 @@ static void irq_handler(struct pt_regs *regs)
 	u32 irqstat = gic_read_iar(1);
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
-	gic_write_eoir(irqstat);
+	gic_write_eoir(irqstat, 1);
 
 	if (irqnr == pl031_irq) {
 		report("  RTC RIS == 1", readl(&pl031->ris) == 1);
diff --git a/arm/timer.c b/arm/timer.c
index e5cc3b4..642e715 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -154,7 +154,7 @@ static void irq_handler(struct pt_regs *regs)
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr != GICC_INT_SPURIOUS)
-		gic_write_eoir(irqstat);
+		gic_write_eoir(irqstat, 1);
 
 	if (irqnr == PPI(vtimer_info.irq)) {
 		info = &vtimer_info;
diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
index 52e7bba..163008d 100644
--- a/lib/arm/asm/arch_gicv3.h
+++ b/lib/arm/asm/arch_gicv3.h
@@ -17,6 +17,7 @@
 #define ICC_PMR				__ACCESS_CP15(c4, 0, c6, 0)
 #define ICC_SGI1R			__ACCESS_CP15_64(0, c12)
 #define ICC_IAR0			__ACCESS_CP15(c12, 0,  c8, 0)
+#define ICC_EOIR0			__ACCESS_CP15(c12, 0,  c8, 1)
 #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
 #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
 #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
@@ -44,9 +45,12 @@ static inline u32 gicv3_read_iar(int group)
 	return irqstat;
 }
 
-static inline void gicv3_write_eoir(u32 irq)
+static inline void gicv3_write_eoir(u32 irq, int group)
 {
-	write_sysreg(irq, ICC_EOIR1);
+	if (group == 0)
+		write_sysreg(irq, ICC_EOIR0);
+	else
+		write_sysreg(irq, ICC_EOIR1);
 	isb();
 }
 
diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
index d50c610..b57ee35 100644
--- a/lib/arm/asm/gic-v2.h
+++ b/lib/arm/asm/gic-v2.h
@@ -34,7 +34,7 @@ extern int gicv2_init(void);
 extern void gicv2_enable_defaults(void);
 extern u32 gicv2_read_iar(int group);
 extern u32 gicv2_iar_irqnr(u32 iar);
-extern void gicv2_write_eoir(u32 irqstat);
+extern void gicv2_write_eoir(u32 irqstat, int group);
 extern void gicv2_ipi_send_single(int irq, int cpu);
 extern void gicv2_ipi_send_mask(int irq, const cpumask_t *dest);
 
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index ca19110..210e07a 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -71,7 +71,7 @@ extern int gicv3_init(void);
 extern void gicv3_enable_defaults(void);
 extern u32 gicv3_read_iar(int group);
 extern u32 gicv3_iar_irqnr(u32 iar);
-extern void gicv3_write_eoir(u32 irqstat);
+extern void gicv3_write_eoir(u32 irqstat, int group);
 extern void gicv3_ipi_send_single(int irq, int cpu);
 extern void gicv3_ipi_send_mask(int irq, const cpumask_t *dest);
 extern void gicv3_set_redist_base(size_t stride);
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 09663e7..c4fedd6 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -70,7 +70,7 @@ extern void gic_enable_defaults(void);
 extern int gic_version(void);
 extern u32 gic_read_iar(int group);
 extern u32 gic_iar_irqnr(u32 iar);
-extern void gic_write_eoir(u32 irqstat);
+extern void gic_write_eoir(u32 irqstat, int group);
 extern void gic_ipi_send_single(int irq, int cpu);
 extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
 
diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
index b60967e..d2af01e 100644
--- a/lib/arm/gic-v2.c
+++ b/lib/arm/gic-v2.c
@@ -37,7 +37,7 @@ u32 gicv2_iar_irqnr(u32 iar)
 	return iar & GICC_IAR_INT_ID_MASK;
 }
 
-void gicv2_write_eoir(u32 irqstat)
+void gicv2_write_eoir(u32 irqstat, int group)
 {
 	writel(irqstat, gicv2_cpu_base() + GICC_EOIR);
 }
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index b51eff5..b9e64b9 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -14,7 +14,7 @@ struct gic_common_ops {
 	void (*enable_defaults)(void);
 	u32 (*read_iar)(int group);
 	u32 (*iar_irqnr)(u32 iar);
-	void (*write_eoir)(u32 irqstat);
+	void (*write_eoir)(u32 irqstat, int group);
 	void (*ipi_send_single)(int irq, int cpu);
 	void (*ipi_send_mask)(int irq, const cpumask_t *dest);
 };
@@ -129,10 +129,10 @@ u32 gic_iar_irqnr(u32 iar)
 	return gic_common_ops->iar_irqnr(iar);
 }
 
-void gic_write_eoir(u32 irqstat)
+void gic_write_eoir(u32 irqstat, int group)
 {
 	assert(gic_common_ops && gic_common_ops->write_eoir);
-	gic_common_ops->write_eoir(irqstat);
+	gic_common_ops->write_eoir(irqstat, group);
 }
 
 void gic_ipi_send_single(int irq, int cpu)
diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
index 876e1fc..972b97e 100644
--- a/lib/arm64/asm/arch_gicv3.h
+++ b/lib/arm64/asm/arch_gicv3.h
@@ -12,6 +12,7 @@
 
 #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
 #define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
+#define ICC_EOIR0_EL1			sys_reg(3, 0, 12, 8, 1)
 #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
 #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
 #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
@@ -52,9 +53,14 @@ static inline u32 gicv3_read_iar(int group)
 	return (u64)irqstat;
 }
 
-static inline void gicv3_write_eoir(u32 irq)
+static inline void gicv3_write_eoir(u32 irq, int group)
 {
-	asm volatile("msr_s " xstr(ICC_EOIR1_EL1) ", %0" : : "r" ((u64)irq));
+	if (group == 0)
+		asm volatile("msr_s " xstr(ICC_EOIR0_EL1) ", %0"
+			     : : "r" ((u64)irq));
+	else
+		asm volatile("msr_s " xstr(ICC_EOIR1_EL1) ", %0"
+			     : : "r" ((u64)irq));
 	isb();
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 14/17] arm: gic: Prepare for receiving GIC group 0 interrupts via FIQs

[Andre Przywara]

To differentiate between the two interrupt groups, we will configure
group 0 to be delivered as FIQs, while group 1 interrupts still use the
IRQ "pin".
For this we need to teach kvm-unit-tests to deal with FIQs, also need to
tell the VGIC to deliver FIQs. This requires some bits here and there to
be set, which are annoyingly different between GICv2 and GICv3.

Add the required code in the GIC library to easily enable FIQ delivery
later on, in gic.c.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 lib/arm/asm/arch_gicv3.h   |  7 +++++++
 lib/arm/asm/gic-v2.h       |  7 ++++++-
 lib/arm/asm/processor.h    | 10 ++++++++++
 lib/arm/gic-v2.c           | 34 +++++++++++++++++++++++++++++++++-
 lib/arm64/asm/arch_gicv3.h |  9 +++++++++
 lib/arm64/asm/processor.h  | 10 ++++++++++
 lib/arm64/processor.c      |  2 ++
 7 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
index 163008d..33fd75c 100644
--- a/lib/arm/asm/arch_gicv3.h
+++ b/lib/arm/asm/arch_gicv3.h
@@ -20,6 +20,7 @@
 #define ICC_EOIR0			__ACCESS_CP15(c12, 0,  c8, 1)
 #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
 #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
+#define ICC_IGRPEN0			__ACCESS_CP15(c12, 0, c12, 6)
 #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
 
 static inline void gicv3_write_pmr(u32 val)
@@ -54,6 +55,12 @@ static inline void gicv3_write_eoir(u32 irq, int group)
 	isb();
 }
 
+static inline void gicv3_write_grpen0(u32 val)
+{
+	write_sysreg(val, ICC_IGRPEN0);
+	isb();
+}
+
 static inline void gicv3_write_grpen1(u32 val)
 {
 	write_sysreg(val, ICC_IGRPEN1);
diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
index b57ee35..ed083ea 100644
--- a/lib/arm/asm/gic-v2.h
+++ b/lib/arm/asm/gic-v2.h
@@ -14,7 +14,10 @@
 
 #define GICD_ENABLE			0x1
 
-#define GICC_ENABLE			0x1
+#define GICC_GRP0_ENABLE		0x1
+#define GICC_GRP1_ENABLE		0x2
+#define GICC_ACKCTL			0x4
+#define GICC_FIQEN			0x8
 #define GICC_IAR_INT_ID_MASK		0x3ff
 
 #ifndef __ASSEMBLY__
@@ -32,6 +35,8 @@ extern struct gicv2_data gicv2_data;
 
 extern int gicv2_init(void);
 extern void gicv2_enable_defaults(void);
+extern void gicv2_enable_group1(bool enable);
+extern void gicv2_enable_fiq(bool enable);
 extern u32 gicv2_read_iar(int group);
 extern u32 gicv2_iar_irqnr(u32 iar);
 extern void gicv2_write_eoir(u32 irqstat, int group);
diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index a8c4628..6b5dd1e 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -35,6 +35,16 @@ static inline unsigned long current_cpsr(void)
 
 #define current_mode() (current_cpsr() & MODE_MASK)
 
+static inline void local_fiq_enable(void)
+{
+	asm volatile("cpsie f" : : : "memory", "cc");
+}
+
+static inline void local_fiq_disable(void)
+{
+	asm volatile("cpsid f" : : : "memory", "cc");
+}
+
 static inline void local_irq_enable(void)
 {
 	asm volatile("cpsie i" : : : "memory", "cc");
diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
index d2af01e..360aaa3 100644
--- a/lib/arm/gic-v2.c
+++ b/lib/arm/gic-v2.c
@@ -23,7 +23,39 @@ void gicv2_enable_defaults(void)
 	writel(GICD_ENABLE, dist + GICD_CTLR);
 
 	writel(GICC_INT_PRI_THRESHOLD, cpu_base + GICC_PMR);
-	writel(GICC_ENABLE, cpu_base + GICC_CTLR);
+	writel(GICC_GRP0_ENABLE, cpu_base + GICC_CTLR);
+}
+
+void gicv2_enable_fiq(bool enable)
+{
+	void *cpu_base = gicv2_cpu_base();
+	u32 reg = readl(cpu_base + GICC_CTLR);
+
+	if (enable) {
+		reg |= GICC_GRP0_ENABLE;
+		reg |= GICC_FIQEN;
+	} else {
+		reg &= ~GICC_GRP0_ENABLE;
+		reg &= ~GICC_FIQEN;
+	}
+
+	writel(reg, cpu_base + GICC_CTLR);
+}
+
+void gicv2_enable_group1(bool enable)
+{
+	void *cpu_base = gicv2_cpu_base();
+	u32 reg = readl(cpu_base + GICC_CTLR);
+
+	if (enable) {
+		reg |= GICC_GRP1_ENABLE;
+		reg |= GICC_ACKCTL;
+	} else {
+		reg &= ~GICC_GRP1_ENABLE;
+		reg &= ~GICC_ACKCTL;
+	}
+
+	writel(reg, cpu_base + GICC_CTLR);
 }
 
 u32 gicv2_read_iar(int group)
diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
index 972b97e..6938bc5 100644
--- a/lib/arm64/asm/arch_gicv3.h
+++ b/lib/arm64/asm/arch_gicv3.h
@@ -14,8 +14,11 @@
 #define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
 #define ICC_EOIR0_EL1			sys_reg(3, 0, 12, 8, 1)
 #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
+#define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
 #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
+#define ICC_EOIR0_EL1			sys_reg(3, 0, 12, 8, 1)
 #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
+#define ICC_GRPEN0_EL1			sys_reg(3, 0, 12, 12, 6)
 #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
 
 #ifndef __ASSEMBLY__
@@ -64,6 +67,12 @@ static inline void gicv3_write_eoir(u32 irq, int group)
 	isb();
 }
 
+static inline void gicv3_write_grpen0(u32 val)
+{
+	asm volatile("msr_s " xstr(ICC_GRPEN0_EL1) ", %0" : : "r" ((u64)val));
+	isb();
+}
+
 static inline void gicv3_write_grpen1(u32 val)
 {
 	asm volatile("msr_s " xstr(ICC_GRPEN1_EL1) ", %0" : : "r" ((u64)val));
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1d9223f..69086e9 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -68,6 +68,16 @@ static inline unsigned long current_level(void)
 	return el & 0xc;
 }
 
+static inline void local_fiq_enable(void)
+{
+	asm volatile("msr daifclr, #1" : : : "memory");
+}
+
+static inline void local_fiq_disable(void)
+{
+	asm volatile("msr daifset, #1" : : : "memory");
+}
+
 static inline void local_irq_enable(void)
 {
 	asm volatile("msr daifclr, #2" : : : "memory");
diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
index 2a024e3..8d7b921 100644
--- a/lib/arm64/processor.c
+++ b/lib/arm64/processor.c
@@ -190,8 +190,10 @@ void vector_handlers_default_init(vector_fn *handlers)
 {
 	handlers[EL1H_SYNC]	= default_vector_sync_handler;
 	handlers[EL1H_IRQ]	= default_vector_irq_handler;
+	handlers[EL1H_FIQ]	= default_vector_irq_handler;
 	handlers[EL0_SYNC_64]	= default_vector_sync_handler;
 	handlers[EL0_IRQ_64]	= default_vector_irq_handler;
+	handlers[EL0_FIQ_64]	= default_vector_irq_handler;
 }
 
 /* Needed to compile with -Wmissing-prototypes */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 15/17] arm: gic: Provide FIQ handler

[Andre Przywara]

When configuring an interrupt as Group 0, we can ask the GIC to deliver
them as a FIQ. For this we need a separate exception handler.

Provide this to be used later.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index c68b5b5..6756850 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -178,6 +178,30 @@ static void irq_handler(struct pt_regs *regs __unused)
 	smp_wmb(); /* pairs with rmb in check_acked */
 }
 
+static inline void fiq_handler(struct pt_regs *regs __unused)
+{
+	u32 irqstat = gic_read_iar(0);
+	u32 irqnr = gic_iar_irqnr(irqstat);
+
+	if (irqnr == GICC_INT_SPURIOUS) {
+		++spurious[smp_processor_id()];
+		smp_wmb();
+		return;
+	}
+
+	gic_write_eoir(irqstat, 0);
+
+	smp_rmb(); /* pairs with wmb in stats_reset */
+	++acked[smp_processor_id()];
+	if (irqnr < GIC_NR_PRIVATE_IRQS) {
+		check_ipi_sender(irqstat);
+		check_irqnr(irqnr, IPI_IRQ);
+	} else {
+		check_irqnr(irqnr, SPI_IRQ);
+	}
+	smp_wmb(); /* pairs with rmb in check_acked */
+}
+
 static void gicv2_ipi_send_self(void)
 {
 	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 16/17] arm: gic: Prepare interrupt statistics for both groups

[Andre Przywara]

The arrays storing information about received interrupts need to
differentiate between IRQs and FIQs, to detect interrupts firing in the
wrong group.

Extend the acked, spurious, bad_sender and bad_irq arrays to get another
dimension, so that we can check the kind of interrupt easily.
The fiq_handler marks its result using index 0, the irq_handler uses
index 1.
Also we add a group parameter to check_acked() and friends, to let them
use the right group.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 78 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 6756850..43a272b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -33,8 +33,8 @@ struct gic {
 };
 
 static struct gic *gic;
-static int acked[NR_CPUS], spurious[NR_CPUS];
-static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
+static int acked[2][NR_CPUS], spurious[2][NR_CPUS];
+static int bad_sender[2][NR_CPUS], bad_irq[2][NR_CPUS];
 static cpumask_t ready;
 
 static void nr_cpu_check(int nr)
@@ -55,14 +55,17 @@ static void stats_reset(void)
 	int i;
 
 	for (i = 0; i < nr_cpus; ++i) {
-		acked[i] = 0;
-		bad_sender[i] = -1;
-		bad_irq[i] = -1;
+		acked[0][i] = 0;
+		acked[1][i] = 0;
+		bad_sender[0][i] = -1;
+		bad_sender[1][i] = -1;
+		bad_irq[0][i] = -1;
+		bad_irq[1][i] = -1;
 	}
 	smp_wmb();
 }
 
-static int check_acked(const char *testname, cpumask_t *mask)
+static int check_acked(const char *testname, cpumask_t *mask, int group)
 {
 	int missing = 0, extra = 0, unexpected = 0;
 	int nr_pass, cpu, i;
@@ -76,17 +79,17 @@ static int check_acked(const char *testname, cpumask_t *mask)
 		for_each_present_cpu(cpu) {
 			smp_rmb();
 			nr_pass += cpumask_test_cpu(cpu, mask) ?
-				acked[cpu] == 1 : acked[cpu] == 0;
+				acked[group][cpu] == 1 : acked[group][cpu] == 0;
 
-			if (bad_sender[cpu] != -1) {
+			if (bad_sender[group][cpu] != -1) {
 				printf("cpu%d received IPI from wrong sender %d\n",
-					cpu, bad_sender[cpu]);
+					cpu, bad_sender[group][cpu]);
 				bad = true;
 			}
 
-			if (bad_irq[cpu] != -1) {
+			if (bad_irq[group][cpu] != -1) {
 				printf("cpu%d received wrong irq %d\n",
-					cpu, bad_irq[cpu]);
+					cpu, bad_irq[group][cpu]);
 				bad = true;
 			}
 		}
@@ -109,12 +112,12 @@ static int check_acked(const char *testname, cpumask_t *mask)
 
 	for_each_present_cpu(cpu) {
 		if (cpumask_test_cpu(cpu, mask)) {
-			if (!acked[cpu])
+			if (!acked[group][cpu])
 				++missing;
-			else if (acked[cpu] > 1)
+			else if (acked[group][cpu] > 1)
 				++extra;
 		} else {
-			if (acked[cpu])
+			if (acked[group][cpu])
 				++unexpected;
 		}
 	}
@@ -132,9 +135,12 @@ static void check_spurious(void)
 
 	smp_rmb();
 	for_each_present_cpu(cpu) {
-		if (spurious[cpu])
-			report_info("WARN: cpu%d got %d spurious interrupts",
-				cpu, spurious[cpu]);
+		if (spurious[0][cpu])
+			report_info("WARN: cpu%d got %d spurious FIQs",
+				    cpu, spurious[0][cpu]);
+		if (spurious[1][cpu])
+			report_info("WARN: cpu%d got %d spurious IRQs",
+				    cpu, spurious[1][cpu]);
 	}
 }
 
@@ -144,14 +150,14 @@ static void check_ipi_sender(u32 irqstat)
 		int src = (irqstat >> 10) & 7;
 
 		if (src != IPI_SENDER)
-			bad_sender[smp_processor_id()] = src;
+			bad_sender[1][smp_processor_id()] = src;
 	}
 }
 
-static void check_irqnr(u32 irqnr, int expected)
+static void check_irqnr(u32 irqnr, int expected, int group)
 {
 	if (irqnr != expected)
-		bad_irq[smp_processor_id()] = irqnr;
+		bad_irq[group][smp_processor_id()] = irqnr;
 }
 
 static void irq_handler(struct pt_regs *regs __unused)
@@ -160,7 +166,7 @@ static void irq_handler(struct pt_regs *regs __unused)
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr == GICC_INT_SPURIOUS) {
-		++spurious[smp_processor_id()];
+		++spurious[1][smp_processor_id()];
 		smp_wmb();
 		return;
 	}
@@ -168,12 +174,12 @@ static void irq_handler(struct pt_regs *regs __unused)
 	gic_write_eoir(irqstat, 1);
 
 	smp_rmb(); /* pairs with wmb in stats_reset */
-	++acked[smp_processor_id()];
+	++acked[1][smp_processor_id()];
 	if (irqnr < GIC_NR_PRIVATE_IRQS) {
 		check_ipi_sender(irqstat);
-		check_irqnr(irqnr, IPI_IRQ);
+		check_irqnr(irqnr, IPI_IRQ, 1);
 	} else {
-		check_irqnr(irqnr, SPI_IRQ);
+		check_irqnr(irqnr, SPI_IRQ, 1);
 	}
 	smp_wmb(); /* pairs with rmb in check_acked */
 }
@@ -184,7 +190,7 @@ static inline void fiq_handler(struct pt_regs *regs __unused)
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr == GICC_INT_SPURIOUS) {
-		++spurious[smp_processor_id()];
+		++spurious[0][smp_processor_id()];
 		smp_wmb();
 		return;
 	}
@@ -192,12 +198,12 @@ static inline void fiq_handler(struct pt_regs *regs __unused)
 	gic_write_eoir(irqstat, 0);
 
 	smp_rmb(); /* pairs with wmb in stats_reset */
-	++acked[smp_processor_id()];
+	++acked[0][smp_processor_id()];
 	if (irqnr < GIC_NR_PRIVATE_IRQS) {
 		check_ipi_sender(irqstat);
-		check_irqnr(irqnr, IPI_IRQ);
+		check_irqnr(irqnr, IPI_IRQ, 0);
 	} else {
-		check_irqnr(irqnr, SPI_IRQ);
+		check_irqnr(irqnr, SPI_IRQ, 0);
 	}
 	smp_wmb(); /* pairs with rmb in check_acked */
 }
@@ -232,7 +238,7 @@ static void ipi_test_self(void)
 	cpumask_clear(&mask);
 	cpumask_set_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_self();
-	check_acked("IPI: self", &mask);
+	check_acked("IPI: self", &mask, 1);
 	report_prefix_pop();
 }
 
@@ -247,7 +253,7 @@ static void ipi_test_smp(void)
 	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
 		cpumask_clear_cpu(i, &mask);
 	gic_ipi_send_mask(IPI_IRQ, &mask);
-	check_acked("IPI: directed", &mask);
+	check_acked("IPI: directed", &mask, 1);
 	report_prefix_pop();
 
 	report_prefix_push("broadcast");
@@ -255,7 +261,7 @@ static void ipi_test_smp(void)
 	cpumask_copy(&mask, &cpu_present_mask);
 	cpumask_clear_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_broadcast();
-	check_acked("IPI: broadcast", &mask);
+	check_acked("IPI: broadcast", &mask, 1);
 	report_prefix_pop();
 }
 
@@ -327,11 +333,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 		writel(val, base + GICD_ICACTIVER);
 
 		smp_rmb(); /* pairs with wmb in stats_reset */
-		++acked[smp_processor_id()];
-		check_irqnr(irqnr, IPI_IRQ);
+		++acked[1][smp_processor_id()];
+		check_irqnr(irqnr, IPI_IRQ, 1);
 		smp_wmb(); /* pairs with rmb in check_acked */
 	} else {
-		++spurious[smp_processor_id()];
+		++spurious[1][smp_processor_id()];
 		smp_wmb();
 	}
 }
@@ -617,7 +623,7 @@ static bool trigger_and_check_spi(const char *test_name,
 		break;
 	}
 
-	ret = (check_acked(test_name, &cpumask) >= 0);
+	ret = (check_acked(test_name, &cpumask, 1) >= 0);
 
 	/* Clean up pending bit in case this IRQ wasn't taken. */
 	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
@@ -643,7 +649,7 @@ static void spi_test_single(void)
 	cpumask_clear(&cpumask);
 	cpumask_set_cpu(cpu, &cpumask);
 	gic_enable_irq(SPI_IRQ);
-	check_acked("now enabled SPI fires", &cpumask);
+	check_acked("now enabled SPI fires", &cpumask, 1);
 }
 
 static void spi_test_smp(void)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[kvm-unit-tests PATCH 17/17] arm: gic: Test Group0 SPIs

[Andre Przywara]

With the newly gained building blocks we can now actually test Group 0
interrupts on our emulated/virtualized GIC.
The least common denominator for the groups usage on both GICv2 and
GICv3 is to configure group 0 interrupts to trigger FIQs, and group 1
interrupts to trigger IRQs.
For testing this we first configure our test SPI to belong to group 0,
then trigger it to see that it is actually delivered as an FIQ, and not as
an IRQ.
The we change the group to become 1, and trigger again, this time
expecting the opposite behaviour.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 43a272b..9942314 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -276,6 +276,22 @@ static void irqs_enable(void)
 	local_irq_enable();
 }
 
+static void fiqs_enable(void)
+{
+#ifdef __arm__
+	install_exception_handler(EXCPTN_FIQ, fiq_handler);
+#else
+	install_irq_handler(EL1H_FIQ, fiq_handler);
+#endif
+	if (gic_version() == 3) {
+		gicv3_write_grpen0(1);
+	} else {
+		gicv2_enable_fiq(true);
+		gicv2_enable_group1(true);
+	}
+	local_fiq_enable();
+}
+
 static void ipi_send(void)
 {
 	irqs_enable();
@@ -598,6 +614,7 @@ static void spi_configure_irq(int irq, int cpu)
 
 #define IRQ_STAT_NONE		0
 #define IRQ_STAT_IRQ		1
+#define IRQ_STAT_FIQ		2
 #define IRQ_STAT_TYPE_MASK	0x3
 #define IRQ_STAT_NO_CLEAR	4
 
@@ -617,14 +634,21 @@ static bool trigger_and_check_spi(const char *test_name,
 	cpumask_clear(&cpumask);
 	switch (irq_stat & IRQ_STAT_TYPE_MASK) {
 	case IRQ_STAT_NONE:
+		ret &= (check_acked(NULL, &cpumask, 0) >= 0);
+		ret &= (check_acked(test_name, &cpumask, 1) >= 0);
 		break;
 	case IRQ_STAT_IRQ:
+		ret &= (check_acked(NULL, &cpumask, 0) >= 0);
+		cpumask_set_cpu(cpu, &cpumask);
+		ret &= (check_acked(test_name, &cpumask, 1) >= 0);
+		break;
+	case IRQ_STAT_FIQ:
+		ret &= (check_acked(NULL, &cpumask, 1) >= 0);
 		cpumask_set_cpu(cpu, &cpumask);
+		ret &= (check_acked(test_name, &cpumask, 0) >= 0);
 		break;
 	}
 
-	ret = (check_acked(test_name, &cpumask, 1) >= 0);
-
 	/* Clean up pending bit in case this IRQ wasn't taken. */
 	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
 		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);
@@ -657,6 +681,9 @@ static void spi_test_smp(void)
 	int cpu;
 	int cores = 1;
 
+	if (nr_cpus > 8)
+		printf("triggering SPIs on all %d cores, takes %d seconds\n",
+		       nr_cpus, (nr_cpus - 1) * 3 / 2);
 	wait_on_ready();
 	for_each_present_cpu(cpu) {
 		if (cpu == smp_processor_id())
@@ -671,6 +698,46 @@ static void spi_test_smp(void)
 }
 
 #define GICD_CTLR_ENABLE_BOTH (GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1)
+#define EXPECT_FIQ	true
+#define EXPECT_IRQ	false
+
+/*
+ * Check whether our SPI interrupt is correctly delivered as an FIQ or as
+ * an IRQ, as configured.
+ * This tries to enable the two groups independently, to check whether
+ * the relation group0->FIQ and group1->IRQ holds.
+ */
+static void gic_check_irq_delivery(void *gicd_base, bool as_fiq)
+{
+	u32 reg = readl(gicd_base + GICD_CTLR) & ~GICD_CTLR_ENABLE_BOTH;
+	int cpu = smp_processor_id();
+
+	/* Check that both groups disabled block the IRQ. */
+	writel(reg, gicd_base + GICD_CTLR);
+	trigger_and_check_spi("no IRQs with both groups disabled",
+			      IRQ_STAT_NONE, cpu);
+
+	/* Check that just the *other* group enabled blocks the IRQ. */
+	if (as_fiq)
+		writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
+	else
+		writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
+	trigger_and_check_spi("no IRQs with just the other group enabled",
+			      IRQ_STAT_NONE, cpu);
+
+	/* Check that just this group enabled fires the IRQ. */
+	if (as_fiq)
+		writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
+	else
+		writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
+	trigger_and_check_spi("just this group enabled",
+			      as_fiq ? IRQ_STAT_FIQ : IRQ_STAT_IRQ, cpu);
+
+	/* Check that both groups enabled fires the IRQ. */
+	writel(reg | GICD_CTLR_ENABLE_BOTH, gicd_base + GICD_CTLR);
+	trigger_and_check_spi("both groups enabled",
+			      as_fiq ? IRQ_STAT_FIQ : IRQ_STAT_IRQ, cpu);
+}
 
 /*
  * Check the security state configuration of the GIC.
@@ -711,6 +778,9 @@ static bool gicv3_check_security(void *gicd_base)
  * Check whether this works as expected (as Linux will not use this feature).
  * We can only verify this state on a GICv3, so we check it there and silently
  * assume it's valid for GICv2.
+ * GICv2 and GICv3 handle the groups differently, but we use the common
+ * denominator (Group0 as FIQ, Group1 as IRQ) and rely on the GIC library for
+ * abstraction.
  */
 static void test_irq_group(void *gicd_base)
 {
@@ -754,6 +824,35 @@ static void test_irq_group(void *gicd_base)
 	gic_set_irq_group(SPI_IRQ, !reg);
 	report("IGROUPR is writable", gic_get_irq_group(SPI_IRQ) != reg);
 	gic_set_irq_group(SPI_IRQ, reg);
+
+	/*
+	 * Configure group 0 interrupts as FIQs, install both an FIQ and IRQ
+	 * handler and allow both types to be delivered to the core.
+	 */
+	irqs_enable();
+	fiqs_enable();
+
+	/* Configure one SPI to be a group0 interrupt. */
+	gic_set_irq_group(SPI_IRQ, 0);
+	spi_configure_irq(SPI_IRQ, smp_processor_id());
+	report_prefix_push("FIQ");
+	gic_check_irq_delivery(gicd_base, EXPECT_FIQ);
+	report_prefix_pop();
+
+	/* Configure the SPI to be a group1 interrupt instead. */
+	gic_set_irq_group(SPI_IRQ, 1);
+	report_prefix_push("IRQ");
+	gic_check_irq_delivery(gicd_base, EXPECT_IRQ);
+	report_prefix_pop();
+
+	/* Reset the IRQ to the default group. */
+	if (is_gicv3)
+		gic_set_irq_group(SPI_IRQ, 1);
+	else
+		gic_set_irq_group(SPI_IRQ, 0);
+	gic_disable_irq(SPI_IRQ);
+
+	report_prefix_pop();
 }
 
 static void spi_send(void)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 01/17] arm: gic: Enable GIC MMIO tests for GICv3 as well

[Alexandru Elisei]

Hi Andre,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> So far the GIC MMIO tests were only enabled for a GICv2 guest. Modern
> machines tend to have a GICv3-only GIC, so can't run those tests.
> It turns out that most GIC distributor registers we test in the unit
> tests are actually the same in GICv3, so we can just enable those tests
> for GICv3 guests as well.
> The only exception is the CPU number in the TYPER register, which is
> only valid in the GICv2 compat mode (ARE=0), which KVM does not support.
> So we protect this test against running on a GICv3 guest.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c            | 13 +++++++++++--
>  arm/unittests.cfg    | 26 ++++++++++++++++++++++----
>  lib/arm/asm/gic-v3.h |  2 ++
>  3 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index adb6aa4..04b3337 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -6,6 +6,7 @@
>   *   + MMIO access tests
>   * GICv3
>   *   + test sending/receiving IPIs
> + *   + MMIO access tests
>   *
>   * Copyright (C) 2016, Red Hat Inc, Andrew Jones <drjones@redhat.com>
>   *
> @@ -496,7 +497,14 @@ static void gic_test_mmio(void)
>  		idreg = gic_dist_base + GICD_ICPIDR2;
>  		break;
>  	case 0x3:
> -		report_abort("GICv3 MMIO tests NYI");
> +		/*
> +		 * We only test generic registers or those affecting
> +		 * SPIs, so don't need to consider the SGI base in
> +		 * the redistributor here.
> +		 */
> +		gic_dist_base = gicv3_dist_base();
> +		idreg = gic_dist_base + GICD_PIDR2;
> +		break;
>  	default:
>  		report_abort("GIC version %d not supported", gic_version());
>  	}
> @@ -505,7 +513,8 @@ static void gic_test_mmio(void)
>  	nr_irqs = GICD_TYPER_IRQS(reg);
>  	report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
>  
> -	test_typer_v2(reg);
> +	if (gic_version() == 0x2)
> +		test_typer_v2(reg);
>  
>  	report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));

More context:

@@ -489,30 +490,38 @@ static void gic_test_mmio(void)
        u32 reg;
        int nr_irqs;
        void *gic_dist_base, *idreg;
 
        switch(gic_version()) {
        case 0x2:
                gic_dist_base = gicv2_dist_base();
                idreg = gic_dist_base + GICD_ICPIDR2;
                break;
        case 0x3:
-               report_abort("GICv3 MMIO tests NYI");
+               /*
+                * We only test generic registers or those affecting
+                * SPIs, so don't need to consider the SGI base in
+                * the redistributor here.
+                */
+               gic_dist_base = gicv3_dist_base();
+               idreg = gic_dist_base + GICD_PIDR2;
+               break;
        default:
                report_abort("GIC version %d not supported", gic_version());
        }
 
        reg = readl(gic_dist_base + GICD_TYPER);
        nr_irqs = GICD_TYPER_IRQS(reg);
        report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
 
-       test_typer_v2(reg);
+       if (gic_version() == 0x2)
+               test_typer_v2(reg);
 
        report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));
 
        report("GICD_TYPER is read-only",
               test_readonly_32(gic_dist_base + GICD_TYPER, false));
        report("GICD_IIDR is read-only",
               test_readonly_32(gic_dist_base + GICD_IIDR, false));
 
        reg = readl(idreg);
        report("ICPIDR2 is read-only", test_readonly_32(idreg, false));

In the case of GICv3, the register is GICD_PIDR2, not ICPIDR2. You can probably
use a different variable to store the identification register name.

>  
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index daeb5a0..12ac142 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -86,28 +86,46 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'ipi'
>  groups = gic
>  
> -[gicv2-mmio]
> +[gicv3-ipi]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'ipi'
> +groups = gic
> +
> +[gicv2-max-mmio]

The renaming is not mentioned in the commit message. If you want to rename these
tests, can you rename them to gic{v2,v3}-mmio-max so they're consistent with the
other test names?

Thanks,
Alex
>  file = gic.flat
>  smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> +[gicv3-max-mmio]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> +
>  [gicv2-mmio-up]
>  file = gic.flat
>  smp = 1
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> +[gicv3-mmio-up]
> +file = gic.flat
> +smp = 1
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> +
>  [gicv2-mmio-3p]
>  file = gic.flat
>  smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> -[gicv3-ipi]
> +[gicv3-mmio-3p]
>  file = gic.flat
> -smp = $MAX_SMP
> -extra_params = -machine gic-version=3 -append 'ipi'
> +smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
> +extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
>  [gicv2-active]
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 347be2f..ed6a5ad 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,8 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_PIDR2			0xffe8
> +
>  /* Re-Distributor registers, offsets from RD_base */
>  #define GICR_TYPER			0x0008
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 02/17] arm: gic: Generalise function names

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> In preparation for adding functions to test SPI interrupts, generalise
> some existing functions dealing with IPIs so far, since most of them
> are actually generic for all kind of interrupts.
> This also reformats the irq_handler() function, to later expand it
> more easily.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 04b3337..a114009 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -135,28 +135,30 @@ static void check_ipi_sender(u32 irqstat)
>  	}
>  }
>  
> -static void check_irqnr(u32 irqnr)
> +static void check_irqnr(u32 irqnr, int expected)
>  {
> -	if (irqnr != IPI_IRQ)
> +	if (irqnr != expected)
>  		bad_irq[smp_processor_id()] = irqnr;
>  }
>  
> -static void ipi_handler(struct pt_regs *regs __unused)
> +static void irq_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
> -	if (irqnr != GICC_INT_SPURIOUS) {
> -		gic_write_eoir(irqstat);
> -		smp_rmb(); /* pairs with wmb in stats_reset */
> -		++acked[smp_processor_id()];
> -		check_ipi_sender(irqstat);
> -		check_irqnr(irqnr);
> -		smp_wmb(); /* pairs with rmb in check_acked */
> -	} else {
> +	if (irqnr == GICC_INT_SPURIOUS) {
>  		++spurious[smp_processor_id()];
>  		smp_wmb();
> +		return;
>  	}
> +
> +	gic_write_eoir(irqstat);
> +
> +	smp_rmb(); /* pairs with wmb in stats_reset */
> +	++acked[smp_processor_id()];
> +	check_ipi_sender(irqstat);
> +	check_irqnr(irqnr, IPI_IRQ);
> +	smp_wmb(); /* pairs with rmb in check_acked */
>  }

I'm not sure this change is necessary. In its current form, it is consistent with
the other irq handler, ipi_clear_active_handler, and your patches add only two
lines: an if statement to check for SPIs and call check_irqnr if true. That is
trivial to integrate in the current handler. Would you care to elaborate why you
think this change is needed?

Thanks,
Alex
>  
>  static void gicv2_ipi_send_self(void)
> @@ -216,20 +218,20 @@ static void ipi_test_smp(void)
>  	report_prefix_pop();
>  }
>  
> -static void ipi_enable(void)
> +static void irqs_enable(void)
>  {
>  	gic_enable_defaults();
>  #ifdef __arm__
> -	install_exception_handler(EXCPTN_IRQ, ipi_handler);
> +	install_exception_handler(EXCPTN_IRQ, irq_handler);
>  #else
> -	install_irq_handler(EL1H_IRQ, ipi_handler);
> +	install_irq_handler(EL1H_IRQ, irq_handler);
>  #endif
>  	local_irq_enable();
>  }
>  
>  static void ipi_send(void)
>  {
> -	ipi_enable();
> +	irqs_enable();
>  	wait_on_ready();
>  	ipi_test_self();
>  	ipi_test_smp();
> @@ -237,9 +239,9 @@ static void ipi_send(void)
>  	exit(report_summary());
>  }
>  
> -static void ipi_recv(void)
> +static void irq_recv(void)
>  {
> -	ipi_enable();
> +	irqs_enable();
>  	cpumask_set_cpu(smp_processor_id(), &ready);
>  	while (1)
>  		wfi();
> @@ -250,7 +252,7 @@ static void ipi_test(void *data __unused)
>  	if (smp_processor_id() == IPI_SENDER)
>  		ipi_send();
>  	else
> -		ipi_recv();
> +		irq_recv();
>  }
>  
>  static struct gic gicv2 = {
> @@ -285,7 +287,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		smp_rmb(); /* pairs with wmb in stats_reset */
>  		++acked[smp_processor_id()];
> -		check_irqnr(irqnr);
> +		check_irqnr(irqnr, IPI_IRQ);
>  		smp_wmb(); /* pairs with rmb in check_acked */
>  	} else {
>  		++spurious[smp_processor_id()];

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 01/17] arm: gic: Enable GIC MMIO tests for GICv3 as well

[Auger Eric]

Hi Andre,

On 11/8/19 3:42 PM, Andre Przywara wrote:
> So far the GIC MMIO tests were only enabled for a GICv2 guest. Modern
> machines tend to have a GICv3-only GIC, so can't run those tests.
> It turns out that most GIC distributor registers we test in the unit
> tests are actually the same in GICv3, so we can just enable those tests
> for GICv3 guests as well.
> The only exception is the CPU number in the TYPER register, which is
> only valid in the GICv2 compat mode (ARE=0), which KVM does not support.
> So we protect this test against running on a GICv3 guest.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c            | 13 +++++++++++--
>  arm/unittests.cfg    | 26 ++++++++++++++++++++++----
>  lib/arm/asm/gic-v3.h |  2 ++
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index adb6aa4..04b3337 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -6,6 +6,7 @@
>   *   + MMIO access tests
>   * GICv3
>   *   + test sending/receiving IPIs
> + *   + MMIO access tests
>   *
>   * Copyright (C) 2016, Red Hat Inc, Andrew Jones <drjones@redhat.com>
>   *
> @@ -496,7 +497,14 @@ static void gic_test_mmio(void)
>  		idreg = gic_dist_base + GICD_ICPIDR2;
>  		break;
>  	case 0x3:
> -		report_abort("GICv3 MMIO tests NYI");
> +		/*
> +		 * We only test generic registers or those affecting
> +		 * SPIs, so don't need to consider the SGI base in
> +		 * the redistributor here.
> +		 */
> +		gic_dist_base = gicv3_dist_base();
> +		idreg = gic_dist_base + GICD_PIDR2;
> +		break;
>  	default:
>  		report_abort("GIC version %d not supported", gic_version());
>  	}
> @@ -505,7 +513,8 @@ static void gic_test_mmio(void)
>  	nr_irqs = GICD_TYPER_IRQS(reg);
>  	report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
>  
> -	test_typer_v2(reg);
> +	if (gic_version() == 0x2)
> +		test_typer_v2(reg);

nit: reports mention ICPIDR2 independently on the version.

	report("ICPIDR2 is read-only", test_readonly_32(idreg, false));
	report_info("value of ICPIDR2: 0x%08x", reg);

>  
>  	report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));
>  
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index daeb5a0..12ac142 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -86,28 +86,46 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'ipi'
>  groups = gic
>  
> -[gicv2-mmio]
> +[gicv3-ipi]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'ipi'
> +groups = gic
> +
> +[gicv2-max-mmio]
>  file = gic.flat
>  smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> +[gicv3-max-mmio]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> +
>  [gicv2-mmio-up]
>  file = gic.flat
>  smp = 1
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> +[gicv3-mmio-up]
> +file = gic.flat
> +smp = 1
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> +
>  [gicv2-mmio-3p]
>  file = gic.flat
>  smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
>  extra_params = -machine gic-version=2 -append 'mmio'
>  groups = gic
>  
> -[gicv3-ipi]
> +[gicv3-mmio-3p]
>  file = gic.flat
> -smp = $MAX_SMP
> -extra_params = -machine gic-version=3 -append 'ipi'
> +smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
why do we keep this smp computation?
> +extra_params = -machine gic-version=2 -append 'mmio'
gic-version=3
>  groups = gic
>  
>  [gicv2-active]
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 347be2f..ed6a5ad 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,8 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_PIDR2			0xffe8
> +
>  /* Re-Distributor registers, offsets from RD_base */
>  #define GICR_TYPER			0x0008
>  
>

Otherwise Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 02/17] arm: gic: Generalise function names

[Auger Eric]

Hi Andre,

On 11/8/19 3:42 PM, Andre Przywara wrote:
> In preparation for adding functions to test SPI interrupts, generalise
> some existing functions dealing with IPIs so far, since most of them
> are actually generic for all kind of interrupts.
> This also reformats the irq_handler() function, to later expand it
> more easily.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 04b3337..a114009 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -135,28 +135,30 @@ static void check_ipi_sender(u32 irqstat)
>  	}
>  }
>  
> -static void check_irqnr(u32 irqnr)
> +static void check_irqnr(u32 irqnr, int expected)
>  {
> -	if (irqnr != IPI_IRQ)
> +	if (irqnr != expected)
>  		bad_irq[smp_processor_id()] = irqnr;
>  }
>  
> -static void ipi_handler(struct pt_regs *regs __unused)
> +static void irq_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
> -	if (irqnr != GICC_INT_SPURIOUS) {
> -		gic_write_eoir(irqstat);
> -		smp_rmb(); /* pairs with wmb in stats_reset */
> -		++acked[smp_processor_id()];
> -		check_ipi_sender(irqstat);
> -		check_irqnr(irqnr);
> -		smp_wmb(); /* pairs with rmb in check_acked */
> -	} else {
> +	if (irqnr == GICC_INT_SPURIOUS) {
>  		++spurious[smp_processor_id()];
>  		smp_wmb();
> +		return;
>  	}
> +
> +	gic_write_eoir(irqstat);
> +
> +	smp_rmb(); /* pairs with wmb in stats_reset */
> +	++acked[smp_processor_id()];
> +	check_ipi_sender(irqstat);
> +	check_irqnr(irqnr, IPI_IRQ);
> +	smp_wmb(); /* pairs with rmb in check_acked */
>  }
>  
>  static void gicv2_ipi_send_self(void)
> @@ -216,20 +218,20 @@ static void ipi_test_smp(void)
>  	report_prefix_pop();
>  }
>  
> -static void ipi_enable(void)
> +static void irqs_enable(void)
>  {
>  	gic_enable_defaults();
>  #ifdef __arm__
> -	install_exception_handler(EXCPTN_IRQ, ipi_handler);
> +	install_exception_handler(EXCPTN_IRQ, irq_handler);
>  #else
> -	install_irq_handler(EL1H_IRQ, ipi_handler);
> +	install_irq_handler(EL1H_IRQ, irq_handler);
>  #endif
>  	local_irq_enable();
>  }
>  
>  static void ipi_send(void)
>  {
> -	ipi_enable();
> +	irqs_enable();
>  	wait_on_ready();
>  	ipi_test_self();
>  	ipi_test_smp();
> @@ -237,9 +239,9 @@ static void ipi_send(void)
>  	exit(report_summary());
>  }
>  
> -static void ipi_recv(void)
> +static void irq_recv(void)
>  {
> -	ipi_enable();
> +	irqs_enable();
>  	cpumask_set_cpu(smp_processor_id(), &ready);
>  	while (1)
>  		wfi();
> @@ -250,7 +252,7 @@ static void ipi_test(void *data __unused)
>  	if (smp_processor_id() == IPI_SENDER)
>  		ipi_send();
>  	else
> -		ipi_recv();
> +		irq_recv();
>  }
>  
>  static struct gic gicv2 = {
> @@ -285,7 +287,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		smp_rmb(); /* pairs with wmb in stats_reset */
>  		++acked[smp_processor_id()];
> -		check_irqnr(irqnr);
> +		check_irqnr(irqnr, IPI_IRQ);
>  		smp_wmb(); /* pairs with rmb in check_acked */
>  	} else {
>  		++spurious[smp_processor_id()];
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> A common theme when accessing per-IRQ parameters in the GIC distributor
> is to set fields of a certain bit width in a range of MMIO registers.
> Examples are the enabled status (one bit per IRQ), the level/edge
> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>
> Add a generic helper function which is able to mask and set the
> respective number of bits, given the IRQ number and the MMIO offset.
> Provide wrappers using this function to easily allow configuring an IRQ.
>
> For now assume that private IRQ numbers always refer to the current CPU.
> In a GICv2 accessing the "other" private IRQs is not easily doable (the
> registers are banked per CPU on the same MMIO address), so we impose the
> same limitation on GICv3, even though those registers are not banked
> there anymore.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  lib/arm/asm/gic-v3.h |  1 +
>  lib/arm/asm/gic.h    |  9 +++++
>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
>
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index ed6a5ad..8cfaed1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,7 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_IROUTER			0x6000
>  #define GICD_PIDR2			0xffe8
>  
>  /* Re-Distributor registers, offsets from RD_base */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a0..21cdb58 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
>  #define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
>  #define GICD_ISPENDR			0x0200
>  #define GICD_ICPENDR			0x0280
>  #define GICD_ISACTIVER			0x0300
> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>  
> +void gic_set_irq_bit(int irq, int offset);
> +void gic_enable_irq(int irq);
> +void gic_disable_irq(int irq);
> +void gic_set_irq_priority(int irq, u8 prio);
> +void gic_set_irq_target(int irq, int cpu);
> +void gic_set_irq_group(int irq, int group);
> +int gic_get_irq_group(int irq);
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 9430116..cf4e811 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>  	gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +enum gic_bit_access {
> +	ACCESS_READ,
> +	ACCESS_SET,
> +	ACCESS_RMW
> +};
> +
> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
> +			      enum gic_bit_access access)
> +{
> +	void *base;
> +	int split = 32 / bits;
> +	int shift = (irq % split) * bits;
> +	u32 reg, mask = ((1U << bits) - 1) << shift;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		base = gicv2_dist_base();
> +		break;
> +	case 3:
> +		if (irq < 32)
> +			base = gicv3_sgi_base();
> +		else
> +			base = gicv3_dist_base();
> +		break;
> +	default:
> +		return 0;
> +	}
> +	base += offset + (irq / split) * 4;

This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
4 = 0. On the other hand, irq * 4 / split = 2.

> +
> +	switch (access) {
> +	case ACCESS_READ:
> +		return (readl(base) & mask) >> shift;
> +	case ACCESS_SET:
> +		reg = 0;
> +		break;
> +	case ACCESS_RMW:
> +		reg = readl(base) & ~mask;
> +		break;
> +	}
> +
> +	writel(reg | ((u32)value << shift), base);
> +
> +	return 0;
> +}
This function looks a bit out of place:
- the function name has a verb in the past tense ('masked'), which makes me think
it should return a bool, but the function actually performs an access to a GIC
register.
- the return value is an u8, but it returns an u32 on a read, because readl
returns an u32.
- the semantics of the function and the return value change based on the access
parameter; worse yet, the return value on a write is completely ignored by the
callers and the value parameter is ignored on reads.

You could split it into separate functions - see below.

> +
> +void gic_set_irq_bit(int irq, int offset)
> +{
> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISENABLER);
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ICENABLER);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +	if (irq < 32)
> +		return;
> +
> +	if (gic_version() == 2) {
> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
> +				    ACCESS_RMW);
> +
> +		return;
> +	}
> +
> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
> +}

The pattern for the public functions in this file is to check that the GIC has
been initialized (assert(gic_common_ops)).

I propose we rewrite the functions like this (compile tested only):

diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 94301169215c..1f5aa7b48828 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
        assert(gic_common_ops && gic_common_ops->ipi_send_mask);
        gic_common_ops->ipi_send_mask(irq, dest);
 }
+
+static void *gic_get_irq_reg(int irq, int offset, int width)
+{
+       void *base;
+
+       switch (gic_version()) {
+       case 2:
+               base = gicv2_dist_base();
+               break;
+       case 3:
+               if (irq < 32)
+                       base = gicv3_sgi_base();
+               else
+                       base = gicv3_dist_base();
+               break;
+       default:
+               return 0;
+       }
+
+       return base + offset + (irq * width / 32);
+}
+
+static void gic_set_irq_field(int irq, int offset, int width, u32 value)
+{
+       void *reg;
+       u32 val;
+       int shift = (irq * width) % 32;
+       u32 mask = ((1U << width) - 1) << shift;
+
+       reg = gic_get_irq_reg(irq, offset, width);
+       val = readl(reg);
+       val = (val & ~mask) | (value << shift);
+       writel(val, reg);
+}
+
+void gic_enable_irq(int irq)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
+}
+
+void gic_disable_irq(int irq)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
+}
+
+void gic_set_irq_priority(int irq, u8 prio)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
+}
+
+void gic_set_irq_target(int irq, int cpu)
+{
+       assert(gic_common_ops);
+
+       if (irq < 32)
+               return;
+
+       if (gic_version() == 2) {
+               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
+               return;
+       }
+
+       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
+}
+
+void gic_set_irq_group(int irq, int group)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
+}
+
+int gic_get_irq_group(int irq)
+{
+       void *reg;
+       u32 val;
+       int shift = irq % 32;
+
+       assert(gic_common_ops);
+       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
+       val = readl(reg);
+
+       return (val >> shift) & 0x1;
+}

A bit more lines of code, but to me more readable. What do you think?



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> For some tests it would be important to check that an IRQ was *not*
> triggered, for instance to test certain masking operations.
>
> Extend the check_added() function to recognise an empty cpumask to
> detect this situation. The timeout duration is reduced, and the "no IRQs

Why is the timeout duration reduced?

> triggered" case is actually reported as a success in this case.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index a114009..eca9188 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
>  	bool bad = false;
> +	bool noirqs = cpumask_empty(mask);
>  
>  	/* Wait up to 5s for all interrupts to be delivered */

This comment needs updating.

> -	for (i = 0; i < 50; ++i) {
> +	for (i = 0; i < (noirqs ? 15 : 50); ++i) {
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  				bad = true;
>  			}
>  		}
> -		if (nr_pass == nr_cpus) {
> +		if (!noirqs && nr_pass == nr_cpus) {

This condition is pretty hard to read - what you are doing here is making sure
that when check_acked tests that no irqs have been received, you do the entire for
loop and wait the entire timeout duration. Did I get that right?

How about this (compile tested only):

+               if (noirqs)
+                       /* Wait for the entire timeout duration. */
+                       continue;
+
                if (nr_pass == nr_cpus) {
                        report("%s", !bad, testname);
                        if (i)

>  			report("%s", !bad, testname);
>  			if (i)
>  				report_info("took more than %d ms", i * 100);
> @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  		}
>  	}
>  
> +	if (noirqs && nr_pass == nr_cpus) {
> +		report("%s", !bad, testname);

bad is true only when bad_sender[cpu] != -1 or bad_irq[cpu] != -1, which only get
set in the irq or ipi handlesr, meaning when you do get an interrupt. If nr_pass
== nr_cpus and noirqs, then you shouldn't have gotten an interrupt. I think it's
safe to write it as report("%s", true, testname). I think a short comment above
explaining why we do this check (timeout expired and we haven't gotten any
interrupts) would also improve readability of the code, but that's up to you.

Thanks,
Alex
> +		return;
> +	}
> +
>  	for_each_present_cpu(cpu) {
>  		if (cpumask_test_cpu(cpu, mask)) {
>  			if (!acked[cpu])

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 05/17] arm: gic: Prepare IRQ handler for handling SPIs

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> So far our IRQ handler routine checks that the received IRQ is actually
> the one SGI (IPI) that we are using for our testing.
>
> To make the IRQ testing routine more versatile, also allow the IRQ to be
> one test SPI (shared interrupt).
> We use the penultimate IRQ of the first SPI group for that purpose.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index eca9188..c909668 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -23,6 +23,7 @@
>  
>  #define IPI_SENDER	1
>  #define IPI_IRQ		1
> +#define SPI_IRQ		(GIC_FIRST_SPI + 30)
>  
>  struct gic {
>  	struct {
> @@ -162,8 +163,12 @@ static void irq_handler(struct pt_regs *regs __unused)
>  
>  	smp_rmb(); /* pairs with wmb in stats_reset */
>  	++acked[smp_processor_id()];
> -	check_ipi_sender(irqstat);
> -	check_irqnr(irqnr, IPI_IRQ);
> +	if (irqnr < GIC_NR_PRIVATE_IRQS) {
> +		check_ipi_sender(irqstat);
> +		check_irqnr(irqnr, IPI_IRQ);
> +	} else {
> +		check_irqnr(irqnr, SPI_IRQ);
> +	}
>  	smp_wmb(); /* pairs with rmb in check_acked */
>  }
>  

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions

[Auger Eric]

Hi Andre

On 11/8/19 3:42 PM, Andre Przywara wrote:
> A common theme when accessing per-IRQ parameters in the GIC distributor
> is to set fields of a certain bit width in a range of MMIO registers.
> Examples are the enabled status (one bit per IRQ), the level/edge
> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
> 
> Add a generic helper function which is able to mask and set the
> respective number of bits, given the IRQ number and the MMIO offset.
> Provide wrappers using this function to easily allow configuring an IRQ.
> 
> For now assume that private IRQ numbers always refer to the current CPU.
> In a GICv2 accessing the "other" private IRQs is not easily doable (the
> registers are banked per CPU on the same MMIO address), so we impose the
> same limitation on GICv3, even though those registers are not banked
> there anymore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  lib/arm/asm/gic-v3.h |  1 +
>  lib/arm/asm/gic.h    |  9 +++++
>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index ed6a5ad..8cfaed1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,7 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_IROUTER			0x6000
>  #define GICD_PIDR2			0xffe8
>  
>  /* Re-Distributor registers, offsets from RD_base */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a0..21cdb58 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
>  #define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
>  #define GICD_ISPENDR			0x0200
>  #define GICD_ICPENDR			0x0280
>  #define GICD_ISACTIVER			0x0300
> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>  
> +void gic_set_irq_bit(int irq, int offset);
> +void gic_enable_irq(int irq);
> +void gic_disable_irq(int irq);
> +void gic_set_irq_priority(int irq, u8 prio);
> +void gic_set_irq_target(int irq, int cpu);
> +void gic_set_irq_group(int irq, int group);
> +int gic_get_irq_group(int irq);
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 9430116..cf4e811 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>  	gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +enum gic_bit_access {
> +	ACCESS_READ,
> +	ACCESS_SET,
> +	ACCESS_RMW
> +};
> +
> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
> +			      enum gic_bit_access access)
> +{
> +	void *base;
> +	int split = 32 / bits;
> +	int shift = (irq % split) * bits;
> +	u32 reg, mask = ((1U << bits) - 1) << shift;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		base = gicv2_dist_base();
> +		break;
> +	case 3:
> +		if (irq < 32)
> +			base = gicv3_sgi_base();
> +		else
> +			base = gicv3_dist_base();
> +		break;
> +	default:
> +		return 0;
> +	}
> +	base += offset + (irq / split) * 4;
> +
> +	switch (access) {
> +	case ACCESS_READ:
> +		return (readl(base) & mask) >> shift;
> +	case ACCESS_SET:
> +		reg = 0;
> +		break;
> +	case ACCESS_RMW:
> +		reg = readl(base) & ~mask;
> +		break;
> +	}
> +
> +	writel(reg | ((u32)value << shift), base);
value & mask may be safer
> +
> +	return 0;
> +}
> +
> +void gic_set_irq_bit(int irq, int offset)
> +{
> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
Why don't we use ACCESS_RMW?
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISENABLER);
here we just want to touch one bit and not erase other bits in the word?
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ICENABLER);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +	if (irq < 32)
> +		return;
> +
> +	if (gic_version() == 2) {
> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
> +				    ACCESS_RMW);
> +
> +		return;
> +	}
> +
> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
> +}
> 
Thanks

Eric


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 06/17] arm: gic: Add simple shared IRQ test

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> So far we were testing the GIC's MMIO interface and IPI delivery.
> Add a simple test to trigger a shared IRQ (SPI), using the ISPENDR
> register in the (emulated) GIC distributor.
> This tests configuration of an IRQ (target CPU) and whether it can be
> properly enabled or disabled.
>
> This is a bit more sophisticated than actually needed at this time,
> but paves the way for extending this to FIQs in the future.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg | 12 +++++++
>  2 files changed, 91 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index c909668..3be76cb 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -546,6 +546,81 @@ static void gic_test_mmio(void)
>  		test_targets(nr_irqs);
>  }
>  
> +static void gic_spi_trigger(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISPENDR);
> +}
> +
> +static void spi_configure_irq(int irq, int cpu)

The function name is slightly misleading - it might be interpreted as configure
irq to target cpu. How about spi_enable_irq?

> +{
> +	gic_set_irq_target(irq, cpu);
> +	gic_set_irq_priority(irq, 0xa0);

It might be worth adding a comment here stating why you have chosen this priority
over priority 0.

> +	gic_enable_irq(irq);
> +}
> +
> +#define IRQ_STAT_NONE		0
> +#define IRQ_STAT_IRQ		1
> +#define IRQ_STAT_TYPE_MASK	0x3
> +#define IRQ_STAT_NO_CLEAR	4
> +
> +/*
> + * Wait for an SPI to fire (or not) on a certain CPU.
> + * Clears the pending bit if requested afterwards.
> + */
> +static void trigger_and_check_spi(const char *test_name,
> +				  unsigned int irq_stat,
> +				  int cpu)
> +{
> +	cpumask_t cpumask;
> +
> +	stats_reset();
> +	gic_spi_trigger(SPI_IRQ);
> +	cpumask_clear(&cpumask);
> +	switch (irq_stat & IRQ_STAT_TYPE_MASK) {
> +	case IRQ_STAT_NONE:
> +		break;
> +	case IRQ_STAT_IRQ:
> +		cpumask_set_cpu(cpu, &cpumask);
> +		break;
> +	}
> +
> +	check_acked(test_name, &cpumask);
> +
> +	/* Clean up pending bit in case this IRQ wasn't taken. */
> +	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
> +		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);

There's only one call site which sets this flag. How about you remove the flag
define and the above two lines of code and replace them with one line of code at
the call site? And if you do that, you can turn the flags into proper enums.

> +}
> +
> +static void spi_test_single(void)
> +{
> +	cpumask_t cpumask;
> +	int cpu = smp_processor_id();
> +
> +	spi_configure_irq(SPI_IRQ, cpu);
> +
> +	trigger_and_check_spi("SPI triggered by CPU write", IRQ_STAT_IRQ, cpu);
> +
> +	gic_disable_irq(SPI_IRQ);
> +	trigger_and_check_spi("disabled SPI does not fire",
> +			      IRQ_STAT_NONE | IRQ_STAT_NO_CLEAR, cpu);
> +
> +	stats_reset();
> +	cpumask_clear(&cpumask);
> +	cpumask_set_cpu(cpu, &cpumask);
> +	gic_enable_irq(SPI_IRQ);
> +	check_acked("now enabled SPI fires", &cpumask);
> +}
> +
> +static void spi_send(void)
> +{
> +	irqs_enable();
> +
> +	spi_test_single();

I suggest your rename this to spi_test_self, to match ipi_test_self.

> +
> +	check_spurious();
> +	exit(report_summary());
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	if (!gic_init()) {
> @@ -577,6 +652,10 @@ int main(int argc, char **argv)
>  		report_prefix_push(argv[1]);
>  		gic_test_mmio();
>  		report_prefix_pop();
> +	} else if (strcmp(argv[1], "irq") == 0) {
> +		report_prefix_push(argv[1]);
> +		spi_send();
> +		report_prefix_pop();
>  	} else {
>  		report_abort("Unknown subtest '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 12ac142..7a78275 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -92,6 +92,18 @@ smp = $MAX_SMP
>  extra_params = -machine gic-version=3 -append 'ipi'
>  groups = gic
>  
> +[gicv2-spi]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> +extra_params = -machine gic-version=2 -append 'irq'

I think 'spi' as a command line parameter makes more sense for the gic{v2,v3}-spi
tests.

Thanks,
Alex
> +groups = gic
> +
> +[gicv3-spi]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'irq'
> +groups = gic
> +
>  [gicv2-max-mmio]
>  file = gic.flat
>  smp = $((($MAX_SMP < 8)?$MAX_SMP:8))

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> For future tests we will need to call check_acked() twice for the same
> interrupt (to test delivery of Group 0 and Group 1 interrupts).
> This should be reported as a single test, so allow check_acked() to be
> called with a "NULL" test name, to suppress output. We report the test
> result via the return value, so the outcome is not lost.
>
> Also this amends the new trigger_and_check_spi() wrapper, to propagate
> the test result to callers of that function.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 3be76cb..63aa9f4 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -62,7 +62,7 @@ static void stats_reset(void)
>  	smp_wmb();
>  }
>  
> -static void check_acked(const char *testname, cpumask_t *mask)
> +static int check_acked(const char *testname, cpumask_t *mask)
>  {
>  	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
> @@ -91,16 +91,20 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  			}
>  		}
>  		if (!noirqs && nr_pass == nr_cpus) {
> -			report("%s", !bad, testname);
> -			if (i)
> -				report_info("took more than %d ms", i * 100);
> -			return;
> +			if (testname) {
> +				report("%s", !bad, testname);
> +				if (i)
> +					report_info("took more than %d ms",
> +						    i * 100);
> +			}
> +			return i * 100;
>  		}
>  	}
>  
>  	if (noirqs && nr_pass == nr_cpus) {
> -		report("%s", !bad, testname);
> -		return;
> +		if (testname)
> +			report("%s", !bad, testname);
> +		return i * 100;
>  	}
>  
>  	for_each_present_cpu(cpu) {
> @@ -115,9 +119,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  		}
>  	}
>  
> -	report("%s", false, testname);
> +	if (testname)
> +		report("%s", false, testname);
>  	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>  		    missing, extra, unexpected);
> +	return -1;
>  }

check_acked is starting to become hard to read. The function itself is rather
inconsistent, as it mixes regular printf's with report_info's. The return value is
also never used:

$ awk '/check_acked\(/ && !/const/' arm/gic.c
    check_acked("IPI: self", &mask);
    check_acked("IPI: directed", &mask);
    check_acked("IPI: broadcast", &mask);

What I'm thinking is that we can rewrite check_acked to return true/false (or
0/1), meaning success or failure, remove the testname parameter, replace the
printfs to report_info, and have the caller do a report based on the value
returned by check_acked.

Rough version, compile tested only, I'm sure it can be improved:

diff --git a/arm/gic.c b/arm/gic.c
index adb6aa464513..5453f2fd3d5f 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -60,11 +60,11 @@ static void stats_reset(void)
        smp_wmb();
 }
 
-static void check_acked(const char *testname, cpumask_t *mask)
+static bool check_acked(cpumask_t *mask)
 {
        int missing = 0, extra = 0, unexpected = 0;
        int nr_pass, cpu, i;
-       bool bad = false;
+       bool success = true;
 
        /* Wait up to 5s for all interrupts to be delivered */
        for (i = 0; i < 50; ++i) {
@@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask)
                                acked[cpu] == 1 : acked[cpu] == 0;
 
                        if (bad_sender[cpu] != -1) {
-                               printf("cpu%d received IPI from wrong sender %d\n",
+                               report_info("cpu%d received IPI from wrong sender
%d\n",
                                        cpu, bad_sender[cpu]);
-                               bad = true;
+                               success = false;
                        }
 
                        if (bad_irq[cpu] != -1) {
-                               printf("cpu%d received wrong irq %d\n",
+                               report_info("cpu%d received wrong irq %d\n",
                                        cpu, bad_irq[cpu]);
-                               bad = true;
+                               success = false;
                        }
                }
                if (nr_pass == nr_cpus) {
-                       report("%s", !bad, testname);
                        if (i)
                                report_info("took more than %d ms", i * 100);
-                       return;
+                       return success;
                }
        }
 
@@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
                }
        }
 
-       report("%s", false, testname);
        report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
                    missing, extra, unexpected);
+       return false;
 }
 
 static void check_spurious(void)
@@ -183,13 +182,11 @@ static void ipi_test_self(void)
 {
        cpumask_t mask;
 
-       report_prefix_push("self");
        stats_reset();
        cpumask_clear(&mask);
        cpumask_set_cpu(smp_processor_id(), &mask);
        gic->ipi.send_self();
-       check_acked("IPI: self", &mask);
-       report_prefix_pop();
+       report("self", check_acked(&mask));
 }
 
 static void ipi_test_smp(void)
@@ -203,7 +200,7 @@ static void ipi_test_smp(void)
        for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
                cpumask_clear_cpu(i, &mask);
        gic_ipi_send_mask(IPI_IRQ, &mask);
-       check_acked("IPI: directed", &mask);
+       report("directed", check_acked(&mask));
        report_prefix_pop();
 
        report_prefix_push("broadcast");
@@ -211,7 +208,7 @@ static void ipi_test_smp(void)
        cpumask_copy(&mask, &cpu_present_mask);
        cpumask_clear_cpu(smp_processor_id(), &mask);
        gic->ipi.send_broadcast();
-       check_acked("IPI: broadcast", &mask);
+       report("broadcast", check_acked(&mask));
        report_prefix_pop();
 }
 
I've removed "IPI" from the report string because the prefixed was already pushed
in main.

Andrew, what do you think? Are we missing something obvious? Do you have a better
idea?

>  static void check_spurious(void)
> @@ -567,11 +573,12 @@ static void spi_configure_irq(int irq, int cpu)
>   * Wait for an SPI to fire (or not) on a certain CPU.
>   * Clears the pending bit if requested afterwards.
>   */
> -static void trigger_and_check_spi(const char *test_name,
> +static bool trigger_and_check_spi(const char *test_name,
>  				  unsigned int irq_stat,
>  				  int cpu)

Why did you change the return value from void to bool if you're not using it
anywhere? If it's because you need it in the next patch (#8), please make the
change there.

Thanks,
Alex
>  {
>  	cpumask_t cpumask;
> +	bool ret = true;
>  
>  	stats_reset();
>  	gic_spi_trigger(SPI_IRQ);
> @@ -584,11 +591,13 @@ static void trigger_and_check_spi(const char *test_name,
>  		break;
>  	}
>  
> -	check_acked(test_name, &cpumask);
> +	ret = (check_acked(test_name, &cpumask) >= 0);
>  
>  	/* Clean up pending bit in case this IRQ wasn't taken. */
>  	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
>  		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);
> +
> +	return ret;
>  }
>  
>  static void spi_test_single(void)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 08/17] arm: gic: Add simple SPI MP test

[Alexandru Elisei]

Hi,

What does MP stand for in the subject? Multi-processor? I think changing it to SMP
makes more sense, as that is also in the test name that you've added.

On 11/8/19 2:42 PM, Andre Przywara wrote:
> Shared Peripheral Interrupts (SPI) can target a specific CPU. Test this
> feature by routing the test SPI to each of the vCPUs, then triggering it
> and confirm its reception on that requested core.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 63aa9f4..304b7b9 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -620,16 +620,45 @@ static void spi_test_single(void)
>  	check_acked("now enabled SPI fires", &cpumask);
>  }
>  
> +static void spi_test_smp(void)
> +{
> +	int cpu;
> +	int cores = 1;
> +
> +	wait_on_ready();
> +	for_each_present_cpu(cpu) {
> +		if (cpu == smp_processor_id())
> +			continue;
> +		spi_configure_irq(SPI_IRQ, cpu);
> +		if (trigger_and_check_spi(NULL, IRQ_STAT_IRQ, cpu))
> +			cores++;
> +		else
> +			report_info("SPI delivery failed on core %d", cpu);
> +	}
> +	report("SPI delievered on all cores", cores == nr_cpus);
> +}
> +
>  static void spi_send(void)
>  {
>  	irqs_enable();
>  
>  	spi_test_single();
>  
> +	if (nr_cpus > 1)
> +		spi_test_smp();
> +
>  	check_spurious();
>  	exit(report_summary());
>  }
>  
> +static void spi_test(void *data __unused)
> +{
> +	if (smp_processor_id() == 0)
> +		spi_send();
> +	else
> +		irq_recv();
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	if (!gic_init()) {
> @@ -663,7 +692,7 @@ int main(int argc, char **argv)
>  		report_prefix_pop();
>  	} else if (strcmp(argv[1], "irq") == 0) {
>  		report_prefix_push(argv[1]);
> -		spi_send();
> +		on_cpus(spi_test, NULL);

This is a bit strange. You call on_cpus here, which means you assume that you have
more than one CPU, but then you check if you have more than one CPU in spi_send,
which gets executed on CPU 0.

How about this instead (compile tested only):

diff --git a/arm/gic.c b/arm/gic.c
index 63aa9f4a9fda..7d2443b06ffa 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -620,12 +620,42 @@ static void spi_test_single(void)
        check_acked("now enabled SPI fires", &cpumask);
 }
 
-static void spi_send(void)
+static void spi_test_smp(void)
 {
-       irqs_enable();
+       int cpu;
+       int cores = 1;
+
+       for_each_present_cpu(cpu) {
+               if (cpu == smp_processor_id())
+                       continue;
+               smp_boot_secondary(cpu, irq_recv);
+       }
+       wait_on_ready();
 
+       for_each_present_cpu(cpu) {
+               if (cpu == smp_processor_id())
+                       continue;
+               spi_configure_irq(SPI_IRQ, cpu);
+               if (trigger_and_check_spi(NULL, IRQ_STAT_IRQ, cpu))
+                       cores++;
+               else
+                       report_info("SPI delivery failed on core %d", cpu);
+       }
+       report("SPI delievered on all cores", cores == nr_cpus);
+}
+
+static void spi_test(void)
+{
+       irqs_enable();
        spi_test_single();
+       if (nr_cpus == 1) {
+               report_skip("At least 2 cpus required to run the SPI SMP test");
+               goto out;
+       }
+
+       spi_test_smp();
 
+out:
        check_spurious();
        exit(report_summary());
 }
@@ -663,7 +693,7 @@ int main(int argc, char **argv)
                report_prefix_pop();
        } else if (strcmp(argv[1], "irq") == 0) {
                report_prefix_push(argv[1]);
-               spi_send();
+               spi_test();
                report_prefix_pop();
        } else {
                report_abort("Unknown subtest '%s'", argv[1]);

What do you think?

Thanks,
Alex
>  		report_prefix_pop();
>  	} else {
>  		report_abort("Unknown subtest '%s'", argv[1]);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions

[Auger Eric]

Hi Alex,

On 11/12/19 1:51 PM, Alexandru Elisei wrote:
> Hi,
> 
> On 11/8/19 2:42 PM, Andre Przywara wrote:
>> A common theme when accessing per-IRQ parameters in the GIC distributor
>> is to set fields of a certain bit width in a range of MMIO registers.
>> Examples are the enabled status (one bit per IRQ), the level/edge
>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>
>> Add a generic helper function which is able to mask and set the
>> respective number of bits, given the IRQ number and the MMIO offset.
>> Provide wrappers using this function to easily allow configuring an IRQ.
>>
>> For now assume that private IRQ numbers always refer to the current CPU.
>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>> registers are banked per CPU on the same MMIO address), so we impose the
>> same limitation on GICv3, even though those registers are not banked
>> there anymore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  lib/arm/asm/gic-v3.h |  1 +
>>  lib/arm/asm/gic.h    |  9 +++++
>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 100 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>> index ed6a5ad..8cfaed1 100644
>> --- a/lib/arm/asm/gic-v3.h
>> +++ b/lib/arm/asm/gic-v3.h
>> @@ -23,6 +23,7 @@
>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>  
>> +#define GICD_IROUTER			0x6000
>>  #define GICD_PIDR2			0xffe8
>>  
>>  /* Re-Distributor registers, offsets from RD_base */
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index 1fc10a0..21cdb58 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -15,6 +15,7 @@
>>  #define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>> +#define GICD_ICENABLER			0x0180
>>  #define GICD_ISPENDR			0x0200
>>  #define GICD_ICPENDR			0x0280
>>  #define GICD_ISACTIVER			0x0300
>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>  extern void gic_ipi_send_single(int irq, int cpu);
>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>  
>> +void gic_set_irq_bit(int irq, int offset);
>> +void gic_enable_irq(int irq);
>> +void gic_disable_irq(int irq);
>> +void gic_set_irq_priority(int irq, u8 prio);
>> +void gic_set_irq_target(int irq, int cpu);
>> +void gic_set_irq_group(int irq, int group);
>> +int gic_get_irq_group(int irq);
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* _ASMARM_GIC_H_ */
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 9430116..cf4e811 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +enum gic_bit_access {
>> +	ACCESS_READ,
>> +	ACCESS_SET,
>> +	ACCESS_RMW
>> +};
>> +
>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>> +			      enum gic_bit_access access)
>> +{
>> +	void *base;
>> +	int split = 32 / bits;
>> +	int shift = (irq % split) * bits;
>> +	u32 reg, mask = ((1U << bits) - 1) << shift;
>> +
>> +	switch (gic_version()) {
>> +	case 2:
>> +		base = gicv2_dist_base();
>> +		break;
>> +	case 3:
>> +		if (irq < 32)
>> +			base = gicv3_sgi_base();
>> +		else
>> +			base = gicv3_dist_base();
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +	base += offset + (irq / split) * 4;
> 
> This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
> 4 = 0. On the other hand, irq * 4 / split = 2.

I think that's correct. if bits = 4 this means there are 8 of such
fields in a word and the field corresponding to irq=4 is indeed located
in word 0.

Thanks

Eric
> 
>> +
>> +	switch (access) {
>> +	case ACCESS_READ:
>> +		return (readl(base) & mask) >> shift;
>> +	case ACCESS_SET:
>> +		reg = 0;
>> +		break;
>> +	case ACCESS_RMW:
>> +		reg = readl(base) & ~mask;
>> +		break;
>> +	}
>> +
>> +	writel(reg | ((u32)value << shift), base);
>> +
>> +	return 0;
>> +}
> This function looks a bit out of place:
> - the function name has a verb in the past tense ('masked'), which makes me think
> it should return a bool, but the function actually performs an access to a GIC
> register.
> - the return value is an u8, but it returns an u32 on a read, because readl
> returns an u32.
> - the semantics of the function and the return value change based on the access
> parameter; worse yet, the return value on a write is completely ignored by the
> callers and the value parameter is ignored on reads.
> 
> You could split it into separate functions - see below.
> 
>> +
>> +void gic_set_irq_bit(int irq, int offset)
>> +{
>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +	if (irq < 32)
>> +		return;
>> +
>> +	if (gic_version() == 2) {
>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>> +				    ACCESS_RMW);
>> +
>> +		return;
>> +	}
>> +
>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>> +}
> 
> The pattern for the public functions in this file is to check that the GIC has
> been initialized (assert(gic_common_ops)).
> 
> I propose we rewrite the functions like this (compile tested only):
> 
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 94301169215c..1f5aa7b48828 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>         assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>         gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +static void *gic_get_irq_reg(int irq, int offset, int width)
> +{
> +       void *base;
> +
> +       switch (gic_version()) {
> +       case 2:
> +               base = gicv2_dist_base();
> +               break;
> +       case 3:
> +               if (irq < 32)
> +                       base = gicv3_sgi_base();
> +               else
> +                       base = gicv3_dist_base();
> +               break;
> +       default:
> +               return 0;
> +       }
> +
> +       return base + offset + (irq * width / 32);
> +}
> +
> +static void gic_set_irq_field(int irq, int offset, int width, u32 value)
> +{
> +       void *reg;
> +       u32 val;
> +       int shift = (irq * width) % 32;
> +       u32 mask = ((1U << width) - 1) << shift;
> +
> +       reg = gic_get_irq_reg(irq, offset, width);
> +       val = readl(reg);
> +       val = (val & ~mask) | (value << shift);
> +       writel(val, reg);
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +       assert(gic_common_ops);
> +
> +       if (irq < 32)
> +               return;
> +
> +       if (gic_version() == 2) {
> +               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
> +               return;
> +       }
> +
> +       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +       void *reg;
> +       u32 val;
> +       int shift = irq % 32;
> +
> +       assert(gic_common_ops);
> +       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
> +       val = readl(reg);
> +
> +       return (val >> shift) & 0x1;
> +}
> 
> A bit more lines of code, but to me more readable. What do you think?
> 
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> The DS (Disable Security) bit in the GICv3 GICD_CTLR register controls
> access to Group 0 interrupts from the non-secure side.
> The KVM VGIC emulation provides a "GIC with a single security state",
> so both groups should be accessible.
> Provide a test to check this bit can be set to one. The current KVM
> emulation should treat this is as RAO/WI (which we also check here). It
> would be architecturally compliant though to have this bit at 0 as well,
> so we refrain from treating different behaviour as a FAIL.

Are we not testing KVM? Why are we not treating a behaviour different than what
KVM should emulate as a fail?

> However we use this as a gateway for further Group 0 IRQ tests.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c            | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/asm/gic-v3.h |  1 +
>  2 files changed, 63 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 304b7b9..c882a24 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -531,6 +531,8 @@ static void gic_test_mmio(void)
>  	reg = readl(gic_dist_base + GICD_TYPER);
>  	nr_irqs = GICD_TYPER_IRQS(reg);
>  	report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
> +	report_info("GIC %s security extension",
> +		reg & (1U << 10) ? "has" : "does not have");
>  
>  	if (gic_version() == 0x2)
>  		test_typer_v2(reg);
> @@ -638,6 +640,60 @@ static void spi_test_smp(void)
>  	report("SPI delievered on all cores", cores == nr_cpus);
>  }
>  
> +/*
> + * Check the security state configuration of the GIC.
> + * Test whether we can switch to a single security state, to test both
> + * group 0 and group 1 interrupts.
> + * Architecturally a GIC can be configured in different ways, so we don't
> + * insist on the current way KVM emulates the GIC.
> + */
> +static bool gicv3_check_security(void *gicd_base)

You don't need gicd_base as a parameter, you know this is called only on a gicv3.

> +{
> +	u32 ctlr = readl(gicd_base + GICD_CTLR);
> +
> +	if (ctlr & GICD_CTLR_DS) {
> +		writel(ctlr & ~GICD_CTLR_DS, gicd_base + GICD_CTLR);
> +		ctlr = readl(gicd_base + GICD_CTLR);
> +		if (!(ctlr & GICD_CTLR_DS))
> +			report_info("GIC allowing two security states");
> +		else
> +			report_info("GIC is one security state only");
> +	} else {
> +		report_info("GIC resets to two security states");
> +	}
> +
> +	writel(ctlr | GICD_CTLR_DS, gicd_base + GICD_CTLR);
> +	ctlr = readl(gicd_base + GICD_CTLR);
> +	report("switching to single security state", ctlr & GICD_CTLR_DS);
> +
> +	/* Group0 delivery only works in single security state. */
> +	return ctlr & GICD_CTLR_DS;
> +}
> +
> +/*
> + * The GIC architecture describes two interrupt groups, group 0 and group 1.
> + * On bare-metal systems, running in non-secure world on a GIC with the
> + * security extensions, there is only one group available: group 1.
> + * However in the kernel KVM emulates a GIC with only one security state,
> + * so both groups are available to guests.
> + * Check whether this works as expected (as Linux will not use this feature).
> + * We can only verify this state on a GICv3, so we check it there and silently
> + * assume it's valid for GICv2.
> + */
> +static void test_irq_group(void *gicd_base)
> +{
> +	bool is_gicv3 = (gic_version() == 3);
> +
> +	report_prefix_push("GROUP");
> +	gic_enable_defaults();

Why is this here if you're only testing GICD_CTLR.DS emulation? Rebase artifact?

> +
> +	if (is_gicv3) {

You can remove the variable is_gicv3 and use gic_version() directly (as you do in
spi_send). Or you can call test_irq_group from spi_send when gic_version is 3 and
drop the check entirely.

> +		/* GICv3 features a bit to read and set the security state. */
> +		if (!gicv3_check_security(gicd_base))
> +			return;
> +	}
> +}
> +
>  static void spi_send(void)
>  {
>  	irqs_enable();
> @@ -647,6 +703,12 @@ static void spi_send(void)
>  	if (nr_cpus > 1)
>  		spi_test_smp();
>  
> +	if (gic_version() == 3)
> +		test_irq_group(gicv3_dist_base());
> +
> +	if (gic_version() == 2)
> +		test_irq_group(gicv2_dist_base());

test_irq_group run an actual test for gicv3 only, I think you can remove the call
when gic_version is 2.

Thanks,
Alex
> +
>  	check_spurious();
>  	exit(report_summary());
>  }
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 8cfaed1..2eaf944 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -19,6 +19,7 @@
>   * group1 enable bits with respect to that view.
>   */
>  #define GICD_CTLR_RWP			(1U << 31)
> +#define GICD_CTLR_DS			(1U << 6)
>  #define GICD_CTLR_ARE_NS		(1U << 4)
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 10/17] arm: gic: Check for writable IGROUPR registers

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> When both groups are avaiable to the non-secure side, the MMIO group
> registers need to be writable, so that the group that an IRQ belongs to
> can be programmed.
>
> Check that the group can be flipped, after having established that both
> groups are usable.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index c882a24..485ca4f 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -683,6 +683,7 @@ static bool gicv3_check_security(void *gicd_base)
>  static void test_irq_group(void *gicd_base)
>  {
>  	bool is_gicv3 = (gic_version() == 3);
> +	u32 reg;

The return value for gic_get_irq_group is an int, not a u32. Also, maybe it should
be named group instead, so it's clear what it represents.

>  
>  	report_prefix_push("GROUP");
>  	gic_enable_defaults();
> @@ -692,6 +693,16 @@ static void test_irq_group(void *gicd_base)
>  		if (!gicv3_check_security(gicd_base))
>  			return;
>  	}
> +
> +	/*
> +	 * On a security aware GIC in non-secure world the IGROUPR registers
> +	 * are RAZ/WI. KVM emulates a single-security-state GIC, so both
> +	 * groups are available and the IGROUPR registers are writable.
> +	 */
> +	reg = gic_get_irq_group(SPI_IRQ);
> +	gic_set_irq_group(SPI_IRQ, !reg);
> +	report("IGROUPR is writable", gic_get_irq_group(SPI_IRQ) != reg);

This is nitpicking, but from a consistency point of view, shouldn't you check that
the new group is the value that you wrote, meaning the check should be
gic_get_irq_group(SPI_IRQ) == !reg?

Thanks,
Alex
> +	gic_set_irq_group(SPI_IRQ, reg);
>  }
>  
>  static void spi_send(void)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions

[Alexandru Elisei]

Hi,

On 11/12/19 3:53 PM, Auger Eric wrote:
> Hi Alex,
>
> On 11/12/19 1:51 PM, Alexandru Elisei wrote:
>> Hi,
>>
>> On 11/8/19 2:42 PM, Andre Przywara wrote:
>>> A common theme when accessing per-IRQ parameters in the GIC distributor
>>> is to set fields of a certain bit width in a range of MMIO registers.
>>> Examples are the enabled status (one bit per IRQ), the level/edge
>>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>>
>>> Add a generic helper function which is able to mask and set the
>>> respective number of bits, given the IRQ number and the MMIO offset.
>>> Provide wrappers using this function to easily allow configuring an IRQ.
>>>
>>> For now assume that private IRQ numbers always refer to the current CPU.
>>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>>> registers are banked per CPU on the same MMIO address), so we impose the
>>> same limitation on GICv3, even though those registers are not banked
>>> there anymore.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  lib/arm/asm/gic-v3.h |  1 +
>>>  lib/arm/asm/gic.h    |  9 +++++
>>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 100 insertions(+)
>>>
>>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>>> index ed6a5ad..8cfaed1 100644
>>> --- a/lib/arm/asm/gic-v3.h
>>> +++ b/lib/arm/asm/gic-v3.h
>>> @@ -23,6 +23,7 @@
>>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>>  
>>> +#define GICD_IROUTER			0x6000
>>>  #define GICD_PIDR2			0xffe8
>>>  
>>>  /* Re-Distributor registers, offsets from RD_base */
>>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>>> index 1fc10a0..21cdb58 100644
>>> --- a/lib/arm/asm/gic.h
>>> +++ b/lib/arm/asm/gic.h
>>> @@ -15,6 +15,7 @@
>>>  #define GICD_IIDR			0x0008
>>>  #define GICD_IGROUPR			0x0080
>>>  #define GICD_ISENABLER			0x0100
>>> +#define GICD_ICENABLER			0x0180
>>>  #define GICD_ISPENDR			0x0200
>>>  #define GICD_ICPENDR			0x0280
>>>  #define GICD_ISACTIVER			0x0300
>>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>>  extern void gic_ipi_send_single(int irq, int cpu);
>>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>>  
>>> +void gic_set_irq_bit(int irq, int offset);
>>> +void gic_enable_irq(int irq);
>>> +void gic_disable_irq(int irq);
>>> +void gic_set_irq_priority(int irq, u8 prio);
>>> +void gic_set_irq_target(int irq, int cpu);
>>> +void gic_set_irq_group(int irq, int group);
>>> +int gic_get_irq_group(int irq);
>>> +
>>>  #endif /* !__ASSEMBLY__ */
>>>  #endif /* _ASMARM_GIC_H_ */
>>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>>> index 9430116..cf4e811 100644
>>> --- a/lib/arm/gic.c
>>> +++ b/lib/arm/gic.c
>>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>>  }
>>> +
>>> +enum gic_bit_access {
>>> +	ACCESS_READ,
>>> +	ACCESS_SET,
>>> +	ACCESS_RMW
>>> +};
>>> +
>>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>>> +			      enum gic_bit_access access)
>>> +{
>>> +	void *base;
>>> +	int split = 32 / bits;
>>> +	int shift = (irq % split) * bits;
>>> +	u32 reg, mask = ((1U << bits) - 1) << shift;
>>> +
>>> +	switch (gic_version()) {
>>> +	case 2:
>>> +		base = gicv2_dist_base();
>>> +		break;
>>> +	case 3:
>>> +		if (irq < 32)
>>> +			base = gicv3_sgi_base();
>>> +		else
>>> +			base = gicv3_dist_base();
>>> +		break;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +	base += offset + (irq / split) * 4;
>> This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
>> 4 = 0. On the other hand, irq * 4 / split = 2.
> I think that's correct. if bits = 4 this means there are 8 of such
> fields in a word and the field corresponding to irq=4 is indeed located
> in word 0.

You're right, I got confused about the 4. Now I realize that the 4 represents the
size of a register.

Thanks,
Alex
> Thanks
>
> Eric
>>> +
>>> +	switch (access) {
>>> +	case ACCESS_READ:
>>> +		return (readl(base) & mask) >> shift;
>>> +	case ACCESS_SET:
>>> +		reg = 0;
>>> +		break;
>>> +	case ACCESS_RMW:
>>> +		reg = readl(base) & ~mask;
>>> +		break;
>>> +	}
>>> +
>>> +	writel(reg | ((u32)value << shift), base);
>>> +
>>> +	return 0;
>>> +}
>> This function looks a bit out of place:
>> - the function name has a verb in the past tense ('masked'), which makes me think
>> it should return a bool, but the function actually performs an access to a GIC
>> register.
>> - the return value is an u8, but it returns an u32 on a read, because readl
>> returns an u32.
>> - the semantics of the function and the return value change based on the access
>> parameter; worse yet, the return value on a write is completely ignored by the
>> callers and the value parameter is ignored on reads.
>>
>> You could split it into separate functions - see below.
>>
>>> +
>>> +void gic_set_irq_bit(int irq, int offset)
>>> +{
>>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>>> +}
>>> +
>>> +void gic_enable_irq(int irq)
>>> +{
>>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>>> +}
>>> +
>>> +void gic_disable_irq(int irq)
>>> +{
>>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>>> +}
>>> +
>>> +void gic_set_irq_priority(int irq, u8 prio)
>>> +{
>>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>>> +}
>>> +
>>> +void gic_set_irq_target(int irq, int cpu)
>>> +{
>>> +	if (irq < 32)
>>> +		return;
>>> +
>>> +	if (gic_version() == 2) {
>>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>>> +				    ACCESS_RMW);
>>> +
>>> +		return;
>>> +	}
>>> +
>>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>>> +}
>>> +
>>> +void gic_set_irq_group(int irq, int group)
>>> +{
>>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>>> +}
>>> +
>>> +int gic_get_irq_group(int irq)
>>> +{
>>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>>> +}
>> The pattern for the public functions in this file is to check that the GIC has
>> been initialized (assert(gic_common_ops)).
>>
>> I propose we rewrite the functions like this (compile tested only):
>>
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 94301169215c..1f5aa7b48828 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>         assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>         gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +static void *gic_get_irq_reg(int irq, int offset, int width)
>> +{
>> +       void *base;
>> +
>> +       switch (gic_version()) {
>> +       case 2:
>> +               base = gicv2_dist_base();
>> +               break;
>> +       case 3:
>> +               if (irq < 32)
>> +                       base = gicv3_sgi_base();
>> +               else
>> +                       base = gicv3_dist_base();
>> +               break;
>> +       default:
>> +               return 0;
>> +       }
>> +
>> +       return base + offset + (irq * width / 32);
>> +}
>> +
>> +static void gic_set_irq_field(int irq, int offset, int width, u32 value)
>> +{
>> +       void *reg;
>> +       u32 val;
>> +       int shift = (irq * width) % 32;
>> +       u32 mask = ((1U << width) - 1) << shift;
>> +
>> +       reg = gic_get_irq_reg(irq, offset, width);
>> +       val = readl(reg);
>> +       val = (val & ~mask) | (value << shift);
>> +       writel(val, reg);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +       assert(gic_common_ops);
>> +
>> +       if (irq < 32)
>> +               return;
>> +
>> +       if (gic_version() == 2) {
>> +               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
>> +               return;
>> +       }
>> +
>> +       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +       void *reg;
>> +       u32 val;
>> +       int shift = irq % 32;
>> +
>> +       assert(gic_common_ops);
>> +       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
>> +       val = readl(reg);
>> +
>> +       return (val >> shift) & 0x1;
>> +}
>>
>> A bit more lines of code, but to me more readable. What do you think?
>>
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 11/17] arm: gic: Check for validity of both group enable bits

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> The GIC distributor actually supports *two* enable bits, one per
> interrupt group. Linux itself won't care and will only ever use one bit.
> In a VM however we have two groups available, so we should be able to
> flip the two separate enable bits.
>
> Provide tests that try to flip the two available bits and check whether
> they stick.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c            | 21 +++++++++++++++++++++
>  lib/arm/asm/gic-v3.h |  4 ++--
>  lib/arm/gic-v3.c     |  2 +-
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 485ca4f..a0511e5 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -640,6 +640,8 @@ static void spi_test_smp(void)
>  	report("SPI delievered on all cores", cores == nr_cpus);
>  }
>  
> +#define GICD_CTLR_ENABLE_BOTH (GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1)
> +
>  /*
>   * Check the security state configuration of the GIC.
>   * Test whether we can switch to a single security state, to test both
> @@ -694,6 +696,25 @@ static void test_irq_group(void *gicd_base)
>  			return;
>  	}
>  
> +	/* Check whether the group enable bits stick. */
> +	reg = readl(gicd_base + GICD_CTLR);
> +	writel(reg & ~GICD_CTLR_ENABLE_BOTH, gicd_base + GICD_CTLR);
> +	reg = readl(gicd_base + GICD_CTLR);
> +	report("both groups disabled sticks",
> +	       (reg & GICD_CTLR_ENABLE_BOTH) == 0);
> +
> +	reg &= ~GICD_CTLR_ENABLE_BOTH;
> +	writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
> +	reg = readl(gicd_base + GICD_CTLR);
> +	report("group 1 enabled sticks",
> +	       (reg & GICD_CTLR_ENABLE_BOTH) == GICD_CTLR_ENABLE_G1);
> +
> +	reg &= ~GICD_CTLR_ENABLE_BOTH;
> +	writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
> +	reg = readl(gicd_base + GICD_CTLR);
> +	report("group 0 enabled sticks",
> +	       (reg & GICD_CTLR_ENABLE_BOTH) == GICD_CTLR_ENABLE_G0);

How about adding a check that enabling both groups at the same time works?

> +
>  	/*
>  	 * On a security aware GIC in non-secure world the IGROUPR registers
>  	 * are RAZ/WI. KVM emulates a single-security-state GIC, so both
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 2eaf944..0a29610 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -21,8 +21,8 @@
>  #define GICD_CTLR_RWP			(1U << 31)
>  #define GICD_CTLR_DS			(1U << 6)
>  #define GICD_CTLR_ARE_NS		(1U << 4)
> -#define GICD_CTLR_ENABLE_G1A		(1U << 1)
> -#define GICD_CTLR_ENABLE_G1		(1U << 0)
> +#define GICD_CTLR_ENABLE_G1		(1U << 1)
> +#define GICD_CTLR_ENABLE_G0		(1U << 0)

Nice cleanup.

>  
>  #define GICD_IROUTER			0x6000
>  #define GICD_PIDR2			0xffe8
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index feecb5e..d6a5186 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -42,7 +42,7 @@ void gicv3_enable_defaults(void)
>  	writel(0, dist + GICD_CTLR);
>  	gicv3_dist_wait_for_rwp();
>  
> -	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1,
> +	writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1,
>  	       dist + GICD_CTLR);
>  	gicv3_dist_wait_for_rwp();
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 12/17] arm: gic: Change gic_read_iar() to take group parameter

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> Acknowledging a GIC group 0 interrupt requires us to use a different
> system register on GICv3. To allow us to differentiate the two groups
> later, add a group parameter to gic_read_iar(). For GICv2 we can use the
> same CPU interface register to acknowledge group 0 as well, so we ignore
> the parameter here.
>
> For now this is still using group 1 on every caller.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c                  |  4 ++--
>  arm/micro-bench.c          |  2 +-
>  arm/pl031.c                |  2 +-
>  arm/timer.c                |  2 +-
>  lib/arm/asm/arch_gicv3.h   | 11 +++++++++--
>  lib/arm/asm/gic-v2.h       |  2 +-
>  lib/arm/asm/gic-v3.h       |  2 +-
>  lib/arm/asm/gic.h          |  2 +-
>  lib/arm/gic-v2.c           |  3 ++-
>  lib/arm/gic.c              |  6 +++---
>  lib/arm64/asm/arch_gicv3.h | 10 ++++++++--
>  11 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index a0511e5..7be13a6 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -156,7 +156,7 @@ static void check_irqnr(u32 irqnr, int expected)
>  
>  static void irq_handler(struct pt_regs *regs __unused)
>  {
> -	u32 irqstat = gic_read_iar();
> +	u32 irqstat = gic_read_iar(1);
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	if (irqnr == GICC_INT_SPURIOUS) {
> @@ -288,7 +288,7 @@ static struct gic gicv3 = {
>  
>  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  {
> -	u32 irqstat = gic_read_iar();
> +	u32 irqstat = gic_read_iar(1);
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index 4612f41..2bfee68 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -33,7 +33,7 @@ static void ipi_irq_handler(struct pt_regs *regs)
>  {
>  	ipi_ready = false;
>  	ipi_received = true;
> -	gic_write_eoir(gic_read_iar());
> +	gic_write_eoir(gic_read_iar(1));
>  	ipi_ready = true;
>  }
>  
> diff --git a/arm/pl031.c b/arm/pl031.c
> index 5672f36..5be3d76 100644
> --- a/arm/pl031.c
> +++ b/arm/pl031.c
> @@ -134,7 +134,7 @@ static void gic_irq_unmask(void)
>  
>  static void irq_handler(struct pt_regs *regs)
>  {
> -	u32 irqstat = gic_read_iar();
> +	u32 irqstat = gic_read_iar(1);
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	gic_write_eoir(irqstat);
> diff --git a/arm/timer.c b/arm/timer.c
> index 0b808d5..e5cc3b4 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -150,7 +150,7 @@ static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
>  static void irq_handler(struct pt_regs *regs)
>  {
>  	struct timer_info *info;
> -	u32 irqstat = gic_read_iar();
> +	u32 irqstat = gic_read_iar(1);
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	if (irqnr != GICC_INT_SPURIOUS)
> diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
> index 45b6096..52e7bba 100644
> --- a/lib/arm/asm/arch_gicv3.h
> +++ b/lib/arm/asm/arch_gicv3.h
> @@ -16,6 +16,7 @@
>  
>  #define ICC_PMR				__ACCESS_CP15(c4, 0, c6, 0)
>  #define ICC_SGI1R			__ACCESS_CP15_64(0, c12)
> +#define ICC_IAR0			__ACCESS_CP15(c12, 0,  c8, 0)
>  #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
>  #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
>  #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
> @@ -30,9 +31,15 @@ static inline void gicv3_write_sgi1r(u64 val)
>  	write_sysreg(val, ICC_SGI1R);
>  }
>  
> -static inline u32 gicv3_read_iar(void)
> +static inline u32 gicv3_read_iar(int group)
>  {
> -	u32 irqstat = read_sysreg(ICC_IAR1);
> +	u32 irqstat;
> +
> +	if (group == 0)
> +		irqstat = read_sysreg(ICC_IAR0);
> +	else
> +		irqstat = read_sysreg(ICC_IAR1);
> +
>  	dsb(sy);
>  	return irqstat;
>  }
> diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> index 1fcfd43..d50c610 100644
> --- a/lib/arm/asm/gic-v2.h
> +++ b/lib/arm/asm/gic-v2.h
> @@ -32,7 +32,7 @@ extern struct gicv2_data gicv2_data;
>  
>  extern int gicv2_init(void);
>  extern void gicv2_enable_defaults(void);
> -extern u32 gicv2_read_iar(void);
> +extern u32 gicv2_read_iar(int group);
>  extern u32 gicv2_iar_irqnr(u32 iar);
>  extern void gicv2_write_eoir(u32 irqstat);
>  extern void gicv2_ipi_send_single(int irq, int cpu);
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 0a29610..ca19110 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -69,7 +69,7 @@ extern struct gicv3_data gicv3_data;
>  
>  extern int gicv3_init(void);
>  extern void gicv3_enable_defaults(void);
> -extern u32 gicv3_read_iar(void);
> +extern u32 gicv3_read_iar(int group);
>  extern u32 gicv3_iar_irqnr(u32 iar);
>  extern void gicv3_write_eoir(u32 irqstat);
>  extern void gicv3_ipi_send_single(int irq, int cpu);
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 21cdb58..09663e7 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -68,7 +68,7 @@ extern void gic_enable_defaults(void);
>   * below will work with any supported gic version.
>   */
>  extern int gic_version(void);
> -extern u32 gic_read_iar(void);
> +extern u32 gic_read_iar(int group);
>  extern u32 gic_iar_irqnr(u32 iar);
>  extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
> diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
> index dc6a97c..b60967e 100644
> --- a/lib/arm/gic-v2.c
> +++ b/lib/arm/gic-v2.c
> @@ -26,8 +26,9 @@ void gicv2_enable_defaults(void)
>  	writel(GICC_ENABLE, cpu_base + GICC_CTLR);
>  }
>  
> -u32 gicv2_read_iar(void)
> +u32 gicv2_read_iar(int group)
>  {
> +	/* GICv2 acks both group0 and group1 IRQs with the same register. */
>  	return readl(gicv2_cpu_base() + GICC_IAR);
>  }
>  
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index cf4e811..b51eff5 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -12,7 +12,7 @@ struct gicv3_data gicv3_data;
>  
>  struct gic_common_ops {
>  	void (*enable_defaults)(void);
> -	u32 (*read_iar)(void);
> +	u32 (*read_iar)(int group);
>  	u32 (*iar_irqnr)(u32 iar);
>  	void (*write_eoir)(u32 irqstat);
>  	void (*ipi_send_single)(int irq, int cpu);
> @@ -117,10 +117,10 @@ void gic_enable_defaults(void)
>  	gic_common_ops->enable_defaults();
>  }
>  
> -u32 gic_read_iar(void)
> +u32 gic_read_iar(int group)
>  {
>  	assert(gic_common_ops && gic_common_ops->read_iar);
> -	return gic_common_ops->read_iar();
> +	return gic_common_ops->read_iar(group);
>  }
>  
>  u32 gic_iar_irqnr(u32 iar)
> diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
> index a7994ec..876e1fc 100644
> --- a/lib/arm64/asm/arch_gicv3.h
> +++ b/lib/arm64/asm/arch_gicv3.h
> @@ -11,6 +11,7 @@
>  #include <asm/sysreg.h>
>  
>  #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
> +#define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
>  #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
>  #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
>  #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
> @@ -38,10 +39,15 @@ static inline void gicv3_write_sgi1r(u64 val)
>  	asm volatile("msr_s " xstr(ICC_SGI1R_EL1) ", %0" : : "r" (val));
>  }
>  
> -static inline u32 gicv3_read_iar(void)
> +static inline u32 gicv3_read_iar(int group)
>  {
>  	u64 irqstat;
> -	asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
> +
> +	if (group == 0)
> +		asm volatile("mrs_s %0, " xstr(ICC_IAR0_EL1) : "=r" (irqstat));
> +	else
> +		asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
> +
>  	dsb(sy);
>  	return (u64)irqstat;
>  }

I'm not sure this is the best approach. Now every test that happens to use the gic
has to know about interrupt groups. Have you considered implementing the functions
that you need for the test in arm/gic.c? Andrew, what do you think?

Thanks,
Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 14/17] arm: gic: Prepare for receiving GIC group 0 interrupts via FIQs

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> To differentiate between the two interrupt groups, we will configure
> group 0 to be delivered as FIQs, while group 1 interrupts still use the
> IRQ "pin".
> For this we need to teach kvm-unit-tests to deal with FIQs, also need to
> tell the VGIC to deliver FIQs. This requires some bits here and there to
> be set, which are annoyingly different between GICv2 and GICv3.
>
> Add the required code in the GIC library to easily enable FIQ delivery
> later on, in gic.c.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  lib/arm/asm/arch_gicv3.h   |  7 +++++++
>  lib/arm/asm/gic-v2.h       |  7 ++++++-
>  lib/arm/asm/processor.h    | 10 ++++++++++
>  lib/arm/gic-v2.c           | 34 +++++++++++++++++++++++++++++++++-
>  lib/arm64/asm/arch_gicv3.h |  9 +++++++++
>  lib/arm64/asm/processor.h  | 10 ++++++++++
>  lib/arm64/processor.c      |  2 ++
>  7 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
> index 163008d..33fd75c 100644
> --- a/lib/arm/asm/arch_gicv3.h
> +++ b/lib/arm/asm/arch_gicv3.h
> @@ -20,6 +20,7 @@
>  #define ICC_EOIR0			__ACCESS_CP15(c12, 0,  c8, 1)
>  #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
>  #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
> +#define ICC_IGRPEN0			__ACCESS_CP15(c12, 0, c12, 6)
>  #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
>  
>  static inline void gicv3_write_pmr(u32 val)
> @@ -54,6 +55,12 @@ static inline void gicv3_write_eoir(u32 irq, int group)
>  	isb();
>  }
>  
> +static inline void gicv3_write_grpen0(u32 val)
> +{
> +	write_sysreg(val, ICC_IGRPEN0);
> +	isb();
> +}
> +
>  static inline void gicv3_write_grpen1(u32 val)
>  {
>  	write_sysreg(val, ICC_IGRPEN1);
> diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> index b57ee35..ed083ea 100644
> --- a/lib/arm/asm/gic-v2.h
> +++ b/lib/arm/asm/gic-v2.h
> @@ -14,7 +14,10 @@
>  
>  #define GICD_ENABLE			0x1
>  
> -#define GICC_ENABLE			0x1
> +#define GICC_GRP0_ENABLE		0x1
> +#define GICC_GRP1_ENABLE		0x2
> +#define GICC_ACKCTL			0x4
> +#define GICC_FIQEN			0x8
>  #define GICC_IAR_INT_ID_MASK		0x3ff
>  
>  #ifndef __ASSEMBLY__
> @@ -32,6 +35,8 @@ extern struct gicv2_data gicv2_data;
>  
>  extern int gicv2_init(void);
>  extern void gicv2_enable_defaults(void);
> +extern void gicv2_enable_group1(bool enable);
> +extern void gicv2_enable_fiq(bool enable);
>  extern u32 gicv2_read_iar(int group);
>  extern u32 gicv2_iar_irqnr(u32 iar);
>  extern void gicv2_write_eoir(u32 irqstat, int group);
> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
> index a8c4628..6b5dd1e 100644
> --- a/lib/arm/asm/processor.h
> +++ b/lib/arm/asm/processor.h
> @@ -35,6 +35,16 @@ static inline unsigned long current_cpsr(void)
>  
>  #define current_mode() (current_cpsr() & MODE_MASK)
>  
> +static inline void local_fiq_enable(void)
> +{
> +	asm volatile("cpsie f" : : : "memory", "cc");
> +}
> +
> +static inline void local_fiq_disable(void)
> +{
> +	asm volatile("cpsid f" : : : "memory", "cc");
> +}
> +
>  static inline void local_irq_enable(void)
>  {
>  	asm volatile("cpsie i" : : : "memory", "cc");
> diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
> index d2af01e..360aaa3 100644
> --- a/lib/arm/gic-v2.c
> +++ b/lib/arm/gic-v2.c
> @@ -23,7 +23,39 @@ void gicv2_enable_defaults(void)
>  	writel(GICD_ENABLE, dist + GICD_CTLR);
>  
>  	writel(GICC_INT_PRI_THRESHOLD, cpu_base + GICC_PMR);
> -	writel(GICC_ENABLE, cpu_base + GICC_CTLR);
> +	writel(GICC_GRP0_ENABLE, cpu_base + GICC_CTLR);
> +}
> +
> +void gicv2_enable_fiq(bool enable)

This is unexpected - the function to enable the delivery of FIQs is called
gicv2_enable_fiq, but the function to enable the delivery of IRQs is called
gicv2_enable_group1. How about we rename this one to gicv2_enable_group0 and we
state that by convention group 0 interrupts will be delivered as FIQs, so it
matches the behaviour of GICv3?

It's also a bit strange that a function with 'enable' in the name actually has the
opposite behaviour based on a parameter. How about we split it into two functions
without a parameter, one called gicv2_enable_group0 and the other
gicv2_disable_group0? Same for gicv2_enable_group1 -> gicv2_enable_group1 +
gicv2_disable_group1.

Also, have you considered adding similar functions for gicv3
(gicv3_{enable,disable}_group1 and gicv3_{enable,disable}_group0) so we're
consistent across both gic versions?

Thanks,
Alex
> +{
> +	void *cpu_base = gicv2_cpu_base();
> +	u32 reg = readl(cpu_base + GICC_CTLR);
> +
> +	if (enable) {
> +		reg |= GICC_GRP0_ENABLE;
> +		reg |= GICC_FIQEN;
> +	} else {
> +		reg &= ~GICC_GRP0_ENABLE;
> +		reg &= ~GICC_FIQEN;
> +	}
> +
> +	writel(reg, cpu_base + GICC_CTLR);
> +}
> +
> +void gicv2_enable_group1(bool enable)
> +{
> +	void *cpu_base = gicv2_cpu_base();
> +	u32 reg = readl(cpu_base + GICC_CTLR);
> +
> +	if (enable) {
> +		reg |= GICC_GRP1_ENABLE;
> +		reg |= GICC_ACKCTL;
> +	} else {
> +		reg &= ~GICC_GRP1_ENABLE;
> +		reg &= ~GICC_ACKCTL;
> +	}
> +
> +	writel(reg, cpu_base + GICC_CTLR);
>  }
>  
>  u32 gicv2_read_iar(int group)
> diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
> index 972b97e..6938bc5 100644
> --- a/lib/arm64/asm/arch_gicv3.h
> +++ b/lib/arm64/asm/arch_gicv3.h
> @@ -14,8 +14,11 @@
>  #define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
>  #define ICC_EOIR0_EL1			sys_reg(3, 0, 12, 8, 1)
>  #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
> +#define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
>  #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
> +#define ICC_EOIR0_EL1			sys_reg(3, 0, 12, 8, 1)
>  #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
> +#define ICC_GRPEN0_EL1			sys_reg(3, 0, 12, 12, 6)
>  #define ICC_GRPEN1_EL1			sys_reg(3, 0, 12, 12, 7)
>  
>  #ifndef __ASSEMBLY__
> @@ -64,6 +67,12 @@ static inline void gicv3_write_eoir(u32 irq, int group)
>  	isb();
>  }
>  
> +static inline void gicv3_write_grpen0(u32 val)
> +{
> +	asm volatile("msr_s " xstr(ICC_GRPEN0_EL1) ", %0" : : "r" ((u64)val));
> +	isb();
> +}
> +
>  static inline void gicv3_write_grpen1(u32 val)
>  {
>  	asm volatile("msr_s " xstr(ICC_GRPEN1_EL1) ", %0" : : "r" ((u64)val));
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 1d9223f..69086e9 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -68,6 +68,16 @@ static inline unsigned long current_level(void)
>  	return el & 0xc;
>  }
>  
> +static inline void local_fiq_enable(void)
> +{
> +	asm volatile("msr daifclr, #1" : : : "memory");
> +}
> +
> +static inline void local_fiq_disable(void)
> +{
> +	asm volatile("msr daifset, #1" : : : "memory");
> +}
> +
>  static inline void local_irq_enable(void)
>  {
>  	asm volatile("msr daifclr, #2" : : : "memory");
> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
> index 2a024e3..8d7b921 100644
> --- a/lib/arm64/processor.c
> +++ b/lib/arm64/processor.c
> @@ -190,8 +190,10 @@ void vector_handlers_default_init(vector_fn *handlers)
>  {
>  	handlers[EL1H_SYNC]	= default_vector_sync_handler;
>  	handlers[EL1H_IRQ]	= default_vector_irq_handler;
> +	handlers[EL1H_FIQ]	= default_vector_irq_handler;
>  	handlers[EL0_SYNC_64]	= default_vector_sync_handler;
>  	handlers[EL0_IRQ_64]	= default_vector_irq_handler;
> +	handlers[EL0_FIQ_64]	= default_vector_irq_handler;
>  }
>  
>  /* Needed to compile with -Wmissing-prototypes */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 05/17] arm: gic: Prepare IRQ handler for handling SPIs

[Auger Eric]

Hi Andre,

On 11/8/19 3:42 PM, Andre Przywara wrote:
> So far our IRQ handler routine checks that the received IRQ is actually
> the one SGI (IPI) that we are using for our testing.
> 
> To make the IRQ testing routine more versatile, also allow the IRQ to be
> one test SPI (shared interrupt).
> We use the penultimate IRQ of the first SPI group for that purpose.
I don't get the above sentence. What do you mean by group here?
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index eca9188..c909668 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -23,6 +23,7 @@
>  
>  #define IPI_SENDER	1
>  #define IPI_IRQ		1
> +#define SPI_IRQ		(GIC_FIRST_SPI + 30)
>  
>  struct gic {
>  	struct {
> @@ -162,8 +163,12 @@ static void irq_handler(struct pt_regs *regs __unused)
>  
>  	smp_rmb(); /* pairs with wmb in stats_reset */
>  	++acked[smp_processor_id()];
> -	check_ipi_sender(irqstat);
> -	check_irqnr(irqnr, IPI_IRQ);
> +	if (irqnr < GIC_NR_PRIVATE_IRQS) {
> +		check_ipi_sender(irqstat);
> +		check_irqnr(irqnr, IPI_IRQ);
> +	} else {
> +		check_irqnr(irqnr, SPI_IRQ);
I think I would rather have different handlers per test.
I have rebased the ITS series and I use a different LPI handler there.
I think you shouldn't be obliged to hardcode a specific intid in the
handler.

Can't we have
static void setup_irq(handler_t handler)?

Thanks

Eric

> +	}
>  	smp_wmb(); /* pairs with rmb in check_acked */
>  }
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case

[Auger Eric]

Hi,

On 11/12/19 2:26 PM, Alexandru Elisei wrote:
> Hi,
> 
> On 11/8/19 2:42 PM, Andre Przywara wrote:
>> For some tests it would be important to check that an IRQ was *not*
>> triggered, for instance to test certain masking operations.
>>
>> Extend the check_added() function to recognise an empty cpumask to
>> detect this situation. The timeout duration is reduced, and the "no IRQs
> 
> Why is the timeout duration reduced?
> 
>> triggered" case is actually reported as a success in this case.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index a114009..eca9188 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>  	int missing = 0, extra = 0, unexpected = 0;
>>  	int nr_pass, cpu, i;
>>  	bool bad = false;
>> +	bool noirqs = cpumask_empty(mask);
>>  
>>  	/* Wait up to 5s for all interrupts to be delivered */
> 
> This comment needs updating.
> 
>> -	for (i = 0; i < 50; ++i) {
>> +	for (i = 0; i < (noirqs ? 15 : 50); ++i) {
>>  		mdelay(100);
>>  		nr_pass = 0;
>>  		for_each_present_cpu(cpu) {
>> @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>  				bad = true;
>>  			}
>>  		}
>> -		if (nr_pass == nr_cpus) {
>> +		if (!noirqs && nr_pass == nr_cpus) {
> 
> This condition is pretty hard to read - what you are doing here is making sure
> that when check_acked tests that no irqs have been received, you do the entire for
> loop and wait the entire timeout duration. Did I get that right?
> 
> How about this (compile tested only):
> 
> +               if (noirqs)
> +                       /* Wait for the entire timeout duration. */
> +                       continue;
> +
>                 if (nr_pass == nr_cpus) {
>                         report("%s", !bad, testname);
>                         if (i)
> 
>>  			report("%s", !bad, testname);
>>  			if (i>>  				report_info("took more than %d ms", i * 100);
>> @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>  		}
>>  	}
>>  
>> +	if (noirqs && nr_pass == nr_cpus) {
>> +		report("%s", !bad, testname);

This one looks at the result of the last iteration (on timeout).

In case of noirqs I think we should be able to return a failure as soon
as an irq is detected where we do not expect it, without waiting for the
full delay?

Thanks

Eric
> 
> bad is true only when bad_sender[cpu] != -1 or bad_irq[cpu] != -1, which only get
> set in the irq or ipi handlesr, meaning when you do get an interrupt. If nr_pass
> == nr_cpus and noirqs, then you shouldn't have gotten an interrupt. I think it's
> safe to write it as report("%s", true, testname). I think a short comment above
> explaining why we do this check (timeout expired and we haven't gotten any
> interrupts) would also improve readability of the code, but that's up to you.
> 
> Thanks,
> Alex
>> +		return;
>> +	}
>> +
>>  	for_each_present_cpu(cpu) {
>>  		if (cpumask_test_cpu(cpu, mask)) {
>>  			if (!acked[cpu])
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 15/17] arm: gic: Provide FIQ handler

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> When configuring an interrupt as Group 0, we can ask the GIC to deliver
> them as a FIQ. For this we need a separate exception handler.
>
> Provide this to be used later.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index c68b5b5..6756850 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -178,6 +178,30 @@ static void irq_handler(struct pt_regs *regs __unused)
>  	smp_wmb(); /* pairs with rmb in check_acked */
>  }
>  
> +static inline void fiq_handler(struct pt_regs *regs __unused)
> +{
> +	u32 irqstat = gic_read_iar(0);
> +	u32 irqnr = gic_iar_irqnr(irqstat);
> +
> +	if (irqnr == GICC_INT_SPURIOUS) {
> +		++spurious[smp_processor_id()];
> +		smp_wmb();
> +		return;
> +	}
> +
> +	gic_write_eoir(irqstat, 0);
> +
> +	smp_rmb(); /* pairs with wmb in stats_reset */
> +	++acked[smp_processor_id()];
> +	if (irqnr < GIC_NR_PRIVATE_IRQS) {
> +		check_ipi_sender(irqstat);
> +		check_irqnr(irqnr, IPI_IRQ);
> +	} else {
> +		check_irqnr(irqnr, SPI_IRQ);
> +	}
> +	smp_wmb(); /* pairs with rmb in check_acked */
> +}

If I'm not mistaken, this is identical to irq_handler, with the exception that
gic_read_iar and gic_write_eoir take group 0 as argument here. Maybe we can
abstract the common code into a function that takes the group as the argument?
What do you think?

Thanks,
Alex
> +
>  static void gicv2_ipi_send_self(void)
>  {
>  	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 16/17] arm: gic: Prepare interrupt statistics for both groups

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> The arrays storing information about received interrupts need to
> differentiate between IRQs and FIQs, to detect interrupts firing in the
> wrong group.
>
> Extend the acked, spurious, bad_sender and bad_irq arrays to get another
> dimension, so that we can check the kind of interrupt easily.
> The fiq_handler marks its result using index 0, the irq_handler uses
> index 1.
> Also we add a group parameter to check_acked() and friends, to let them
> use the right group.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 78 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 42 insertions(+), 36 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 6756850..43a272b 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -33,8 +33,8 @@ struct gic {
>  };
>  
>  static struct gic *gic;
> -static int acked[NR_CPUS], spurious[NR_CPUS];
> -static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> +static int acked[2][NR_CPUS], spurious[2][NR_CPUS];
> +static int bad_sender[2][NR_CPUS], bad_irq[2][NR_CPUS];
>  static cpumask_t ready;
>  
>  static void nr_cpu_check(int nr)
> @@ -55,14 +55,17 @@ static void stats_reset(void)
>  	int i;
>  
>  	for (i = 0; i < nr_cpus; ++i) {
> -		acked[i] = 0;
> -		bad_sender[i] = -1;
> -		bad_irq[i] = -1;
> +		acked[0][i] = 0;
> +		acked[1][i] = 0;
> +		bad_sender[0][i] = -1;
> +		bad_sender[1][i] = -1;
> +		bad_irq[0][i] = -1;
> +		bad_irq[1][i] = -1;

How about we use defines for the groups instead of 0/1? bad_irq[GIC_GROUP0][i] (or
whatever name you choose) looks more readable to me than bad_irq[0][i].

Thanks,
Alex
>  	}
>  	smp_wmb();
>  }
>  
> -static int check_acked(const char *testname, cpumask_t *mask)
> +static int check_acked(const char *testname, cpumask_t *mask, int group)
>  {
>  	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
> @@ -76,17 +79,17 @@ static int check_acked(const char *testname, cpumask_t *mask)
>  		for_each_present_cpu(cpu) {
>  			smp_rmb();
>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> -				acked[cpu] == 1 : acked[cpu] == 0;
> +				acked[group][cpu] == 1 : acked[group][cpu] == 0;
>  
> -			if (bad_sender[cpu] != -1) {
> +			if (bad_sender[group][cpu] != -1) {
>  				printf("cpu%d received IPI from wrong sender %d\n",
> -					cpu, bad_sender[cpu]);
> +					cpu, bad_sender[group][cpu]);
>  				bad = true;
>  			}
>  
> -			if (bad_irq[cpu] != -1) {
> +			if (bad_irq[group][cpu] != -1) {
>  				printf("cpu%d received wrong irq %d\n",
> -					cpu, bad_irq[cpu]);
> +					cpu, bad_irq[group][cpu]);
>  				bad = true;
>  			}
>  		}
> @@ -109,12 +112,12 @@ static int check_acked(const char *testname, cpumask_t *mask)
>  
>  	for_each_present_cpu(cpu) {
>  		if (cpumask_test_cpu(cpu, mask)) {
> -			if (!acked[cpu])
> +			if (!acked[group][cpu])
>  				++missing;
> -			else if (acked[cpu] > 1)
> +			else if (acked[group][cpu] > 1)
>  				++extra;
>  		} else {
> -			if (acked[cpu])
> +			if (acked[group][cpu])
>  				++unexpected;
>  		}
>  	}
> @@ -132,9 +135,12 @@ static void check_spurious(void)
>  
>  	smp_rmb();
>  	for_each_present_cpu(cpu) {
> -		if (spurious[cpu])
> -			report_info("WARN: cpu%d got %d spurious interrupts",
> -				cpu, spurious[cpu]);
> +		if (spurious[0][cpu])
> +			report_info("WARN: cpu%d got %d spurious FIQs",
> +				    cpu, spurious[0][cpu]);
> +		if (spurious[1][cpu])
> +			report_info("WARN: cpu%d got %d spurious IRQs",
> +				    cpu, spurious[1][cpu]);
>  	}
>  }
>  
> @@ -144,14 +150,14 @@ static void check_ipi_sender(u32 irqstat)
>  		int src = (irqstat >> 10) & 7;
>  
>  		if (src != IPI_SENDER)
> -			bad_sender[smp_processor_id()] = src;
> +			bad_sender[1][smp_processor_id()] = src;
>  	}
>  }
>  
> -static void check_irqnr(u32 irqnr, int expected)
> +static void check_irqnr(u32 irqnr, int expected, int group)
>  {
>  	if (irqnr != expected)
> -		bad_irq[smp_processor_id()] = irqnr;
> +		bad_irq[group][smp_processor_id()] = irqnr;
>  }
>  
>  static void irq_handler(struct pt_regs *regs __unused)
> @@ -160,7 +166,7 @@ static void irq_handler(struct pt_regs *regs __unused)
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	if (irqnr == GICC_INT_SPURIOUS) {
> -		++spurious[smp_processor_id()];
> +		++spurious[1][smp_processor_id()];
>  		smp_wmb();
>  		return;
>  	}
> @@ -168,12 +174,12 @@ static void irq_handler(struct pt_regs *regs __unused)
>  	gic_write_eoir(irqstat, 1);
>  
>  	smp_rmb(); /* pairs with wmb in stats_reset */
> -	++acked[smp_processor_id()];
> +	++acked[1][smp_processor_id()];
>  	if (irqnr < GIC_NR_PRIVATE_IRQS) {
>  		check_ipi_sender(irqstat);
> -		check_irqnr(irqnr, IPI_IRQ);
> +		check_irqnr(irqnr, IPI_IRQ, 1);
>  	} else {
> -		check_irqnr(irqnr, SPI_IRQ);
> +		check_irqnr(irqnr, SPI_IRQ, 1);
>  	}
>  	smp_wmb(); /* pairs with rmb in check_acked */
>  }
> @@ -184,7 +190,7 @@ static inline void fiq_handler(struct pt_regs *regs __unused)
>  	u32 irqnr = gic_iar_irqnr(irqstat);
>  
>  	if (irqnr == GICC_INT_SPURIOUS) {
> -		++spurious[smp_processor_id()];
> +		++spurious[0][smp_processor_id()];
>  		smp_wmb();
>  		return;
>  	}
> @@ -192,12 +198,12 @@ static inline void fiq_handler(struct pt_regs *regs __unused)
>  	gic_write_eoir(irqstat, 0);
>  
>  	smp_rmb(); /* pairs with wmb in stats_reset */
> -	++acked[smp_processor_id()];
> +	++acked[0][smp_processor_id()];
>  	if (irqnr < GIC_NR_PRIVATE_IRQS) {
>  		check_ipi_sender(irqstat);
> -		check_irqnr(irqnr, IPI_IRQ);
> +		check_irqnr(irqnr, IPI_IRQ, 0);
>  	} else {
> -		check_irqnr(irqnr, SPI_IRQ);
> +		check_irqnr(irqnr, SPI_IRQ, 0);
>  	}
>  	smp_wmb(); /* pairs with rmb in check_acked */
>  }
> @@ -232,7 +238,7 @@ static void ipi_test_self(void)
>  	cpumask_clear(&mask);
>  	cpumask_set_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_self();
> -	check_acked("IPI: self", &mask);
> +	check_acked("IPI: self", &mask, 1);
>  	report_prefix_pop();
>  }
>  
> @@ -247,7 +253,7 @@ static void ipi_test_smp(void)
>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>  		cpumask_clear_cpu(i, &mask);
>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> -	check_acked("IPI: directed", &mask);
> +	check_acked("IPI: directed", &mask, 1);
>  	report_prefix_pop();
>  
>  	report_prefix_push("broadcast");
> @@ -255,7 +261,7 @@ static void ipi_test_smp(void)
>  	cpumask_copy(&mask, &cpu_present_mask);
>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_broadcast();
> -	check_acked("IPI: broadcast", &mask);
> +	check_acked("IPI: broadcast", &mask, 1);
>  	report_prefix_pop();
>  }
>  
> @@ -327,11 +333,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  		writel(val, base + GICD_ICACTIVER);
>  
>  		smp_rmb(); /* pairs with wmb in stats_reset */
> -		++acked[smp_processor_id()];
> -		check_irqnr(irqnr, IPI_IRQ);
> +		++acked[1][smp_processor_id()];
> +		check_irqnr(irqnr, IPI_IRQ, 1);
>  		smp_wmb(); /* pairs with rmb in check_acked */
>  	} else {
> -		++spurious[smp_processor_id()];
> +		++spurious[1][smp_processor_id()];
>  		smp_wmb();
>  	}
>  }
> @@ -617,7 +623,7 @@ static bool trigger_and_check_spi(const char *test_name,
>  		break;
>  	}
>  
> -	ret = (check_acked(test_name, &cpumask) >= 0);
> +	ret = (check_acked(test_name, &cpumask, 1) >= 0);
>  
>  	/* Clean up pending bit in case this IRQ wasn't taken. */
>  	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
> @@ -643,7 +649,7 @@ static void spi_test_single(void)
>  	cpumask_clear(&cpumask);
>  	cpumask_set_cpu(cpu, &cpumask);
>  	gic_enable_irq(SPI_IRQ);
> -	check_acked("now enabled SPI fires", &cpumask);
> +	check_acked("now enabled SPI fires", &cpumask, 1);
>  }
>  
>  static void spi_test_smp(void)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 17/17] arm: gic: Test Group0 SPIs

[Alexandru Elisei]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> With the newly gained building blocks we can now actually test Group 0
> interrupts on our emulated/virtualized GIC.
> The least common denominator for the groups usage on both GICv2 and
> GICv3 is to configure group 0 interrupts to trigger FIQs, and group 1
> interrupts to trigger IRQs.
> For testing this we first configure our test SPI to belong to group 0,
> then trigger it to see that it is actually delivered as an FIQ, and not as
> an IRQ.
> The we change the group to become 1, and trigger again, this time
> expecting the opposite behaviour.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 2 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 43a272b..9942314 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -276,6 +276,22 @@ static void irqs_enable(void)
>  	local_irq_enable();
>  }
>  
> +static void fiqs_enable(void)
> +{
> +#ifdef __arm__
> +	install_exception_handler(EXCPTN_FIQ, fiq_handler);
> +#else
> +	install_irq_handler(EL1H_FIQ, fiq_handler);
> +#endif
> +	if (gic_version() == 3) {
> +		gicv3_write_grpen0(1);
> +	} else {
> +		gicv2_enable_fiq(true);
> +		gicv2_enable_group1(true);

Why do we enable group 1 here for GICv2? The commit message says that group 1
interrupts will be delivered as IRQs.

> +	}
> +	local_fiq_enable();
> +}
> +
>  static void ipi_send(void)
>  {
>  	irqs_enable();
> @@ -598,6 +614,7 @@ static void spi_configure_irq(int irq, int cpu)
>  
>  #define IRQ_STAT_NONE		0
>  #define IRQ_STAT_IRQ		1
> +#define IRQ_STAT_FIQ		2
>  #define IRQ_STAT_TYPE_MASK	0x3
>  #define IRQ_STAT_NO_CLEAR	4
>  
> @@ -617,14 +634,21 @@ static bool trigger_and_check_spi(const char *test_name,
>  	cpumask_clear(&cpumask);
>  	switch (irq_stat & IRQ_STAT_TYPE_MASK) {
>  	case IRQ_STAT_NONE:
> +		ret &= (check_acked(NULL, &cpumask, 0) >= 0);
> +		ret &= (check_acked(test_name, &cpumask, 1) >= 0);
>  		break;
>  	case IRQ_STAT_IRQ:
> +		ret &= (check_acked(NULL, &cpumask, 0) >= 0);
> +		cpumask_set_cpu(cpu, &cpumask);
> +		ret &= (check_acked(test_name, &cpumask, 1) >= 0);
> +		break;
> +	case IRQ_STAT_FIQ:
> +		ret &= (check_acked(NULL, &cpumask, 1) >= 0);
>  		cpumask_set_cpu(cpu, &cpumask);
> +		ret &= (check_acked(test_name, &cpumask, 0) >= 0);
>  		break;
>  	}
>  
> -	ret = (check_acked(test_name, &cpumask, 1) >= 0);
> -
>  	/* Clean up pending bit in case this IRQ wasn't taken. */
>  	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
>  		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);
> @@ -657,6 +681,9 @@ static void spi_test_smp(void)
>  	int cpu;
>  	int cores = 1;
>  
> +	if (nr_cpus > 8)
> +		printf("triggering SPIs on all %d cores, takes %d seconds\n",
> +		       nr_cpus, (nr_cpus - 1) * 3 / 2);

Can you explain how you got the elapsed time for triggering SPIs?

>  	wait_on_ready();
>  	for_each_present_cpu(cpu) {
>  		if (cpu == smp_processor_id())
> @@ -671,6 +698,46 @@ static void spi_test_smp(void)
>  }
>  
>  #define GICD_CTLR_ENABLE_BOTH (GICD_CTLR_ENABLE_G0 | GICD_CTLR_ENABLE_G1)
> +#define EXPECT_FIQ	true
> +#define EXPECT_IRQ	false
> +
> +/*
> + * Check whether our SPI interrupt is correctly delivered as an FIQ or as
> + * an IRQ, as configured.
> + * This tries to enable the two groups independently, to check whether
> + * the relation group0->FIQ and group1->IRQ holds.
> + */
> +static void gic_check_irq_delivery(void *gicd_base, bool as_fiq)

The function also checks for FIQs, as alluded to by the as_fiq parameter; also,
most of the function does different things based on that parameter. I think it
would be cleaner to split it into two functions, gic_check_irq_delivery and
gic_check_fiq_delivery. That way you can have more clearer messages for
trigger_and_check_spi and you get rid of the two defines above. At the very least,
it should be renamed to something more appropriate for what it does, like
gic_check_int_delivery.

> +{
> +	u32 reg = readl(gicd_base + GICD_CTLR) & ~GICD_CTLR_ENABLE_BOTH;
> +	int cpu = smp_processor_id();
> +
> +	/* Check that both groups disabled block the IRQ. */
> +	writel(reg, gicd_base + GICD_CTLR);
> +	trigger_and_check_spi("no IRQs with both groups disabled",
> +			      IRQ_STAT_NONE, cpu);
> +
> +	/* Check that just the *other* group enabled blocks the IRQ. */
> +	if (as_fiq)
> +		writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
> +	else
> +		writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
> +	trigger_and_check_spi("no IRQs with just the other group enabled",
> +			      IRQ_STAT_NONE, cpu);
> +
> +	/* Check that just this group enabled fires the IRQ. */
> +	if (as_fiq)
> +		writel(reg | GICD_CTLR_ENABLE_G0, gicd_base + GICD_CTLR);
> +	else
> +		writel(reg | GICD_CTLR_ENABLE_G1, gicd_base + GICD_CTLR);
> +	trigger_and_check_spi("just this group enabled",
> +			      as_fiq ? IRQ_STAT_FIQ : IRQ_STAT_IRQ, cpu);
> +
> +	/* Check that both groups enabled fires the IRQ. */
> +	writel(reg | GICD_CTLR_ENABLE_BOTH, gicd_base + GICD_CTLR);
> +	trigger_and_check_spi("both groups enabled",
> +			      as_fiq ? IRQ_STAT_FIQ : IRQ_STAT_IRQ, cpu);
> +}
>  
>  /*
>   * Check the security state configuration of the GIC.
> @@ -711,6 +778,9 @@ static bool gicv3_check_security(void *gicd_base)
>   * Check whether this works as expected (as Linux will not use this feature).
>   * We can only verify this state on a GICv3, so we check it there and silently
>   * assume it's valid for GICv2.
> + * GICv2 and GICv3 handle the groups differently, but we use the common
> + * denominator (Group0 as FIQ, Group1 as IRQ) and rely on the GIC library for
> + * abstraction.
>   */
>  static void test_irq_group(void *gicd_base)
>  {
> @@ -754,6 +824,35 @@ static void test_irq_group(void *gicd_base)
>  	gic_set_irq_group(SPI_IRQ, !reg);
>  	report("IGROUPR is writable", gic_get_irq_group(SPI_IRQ) != reg);
>  	gic_set_irq_group(SPI_IRQ, reg);
> +
> +	/*
> +	 * Configure group 0 interrupts as FIQs, install both an FIQ and IRQ
> +	 * handler and allow both types to be delivered to the core.
> +	 */
> +	irqs_enable();
> +	fiqs_enable();
> +
> +	/* Configure one SPI to be a group0 interrupt. */
> +	gic_set_irq_group(SPI_IRQ, 0);
> +	spi_configure_irq(SPI_IRQ, smp_processor_id());
> +	report_prefix_push("FIQ");
> +	gic_check_irq_delivery(gicd_base, EXPECT_FIQ);
> +	report_prefix_pop();
> +
> +	/* Configure the SPI to be a group1 interrupt instead. */
> +	gic_set_irq_group(SPI_IRQ, 1);
> +	report_prefix_push("IRQ");
> +	gic_check_irq_delivery(gicd_base, EXPECT_IRQ);
> +	report_prefix_pop();
> +
> +	/* Reset the IRQ to the default group. */
> +	if (is_gicv3)
> +		gic_set_irq_group(SPI_IRQ, 1);
> +	else
> +		gic_set_irq_group(SPI_IRQ, 0);
> +	gic_disable_irq(SPI_IRQ);
> +
> +	report_prefix_pop();
>  }
>  
>  static void spi_send(void)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call

[Andrew Jones]

On Tue, Nov 12, 2019 at 03:23:04PM +0000, Alexandru Elisei wrote:
> check_acked is starting to become hard to read.

Agreed. check_acked() could probably have some of its subtests factored
out to improve its readability.

> The function itself is rather inconsistent, as it mixes regular
> printf's with report_info's.

Sounds good

> The return value is also never used:
> 
> $ awk '/check_acked\(/ && !/const/' arm/gic.c
>     check_acked("IPI: self", &mask);
>     check_acked("IPI: directed", &mask);
>     check_acked("IPI: broadcast", &mask);

That's good, since it's a void function :-)

> 
> What I'm thinking is that we can rewrite check_acked to return true/false (or
> 0/1), meaning success or failure, remove the testname parameter, replace the
> printfs to report_info, and have the caller do a report based on the value
> returned by check_acked.
> 
> Rough version, compile tested only, I'm sure it can be improved:
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index adb6aa464513..5453f2fd3d5f 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -60,11 +60,11 @@ static void stats_reset(void)
>         smp_wmb();
>  }
>  
> -static void check_acked(const char *testname, cpumask_t *mask)
> +static bool check_acked(cpumask_t *mask)

We have several check_* functions in arm/gic.c, and they're all void
functions. Changing this one to a bool would be inconsistent, but
maybe that consistency isn't that important, or maybe they should all
be bool?

>  {
>         int missing = 0, extra = 0, unexpected = 0;
>         int nr_pass, cpu, i;
> -       bool bad = false;
> +       bool success = true;
>  
>         /* Wait up to 5s for all interrupts to be delivered */
>         for (i = 0; i < 50; ++i) {
> @@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask)
>                                 acked[cpu] == 1 : acked[cpu] == 0;
>  
>                         if (bad_sender[cpu] != -1) {
> -                               printf("cpu%d received IPI from wrong sender %d\n",
> +                               report_info("cpu%d received IPI from wrong sender
> %d\n",
>                                         cpu, bad_sender[cpu]);
> -                               bad = true;
> +                               success = false;
>                         }
>  
>                         if (bad_irq[cpu] != -1) {
> -                               printf("cpu%d received wrong irq %d\n",
> +                               report_info("cpu%d received wrong irq %d\n",
>                                         cpu, bad_irq[cpu]);
> -                               bad = true;
> +                               success = false;
>                         }
>                 }
>                 if (nr_pass == nr_cpus) {
> -                       report("%s", !bad, testname);
>                         if (i)
>                                 report_info("took more than %d ms", i * 100);
> -                       return;
> +                       return success;
>                 }
>         }
>  
> @@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
>                 }
>         }
>  
> -       report("%s", false, testname);
>         report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>                     missing, extra, unexpected);
> +       return false;
>  }
>  
>  static void check_spurious(void)
> @@ -183,13 +182,11 @@ static void ipi_test_self(void)
>  {
>         cpumask_t mask;
>  
> -       report_prefix_push("self");
>         stats_reset();
>         cpumask_clear(&mask);
>         cpumask_set_cpu(smp_processor_id(), &mask);
>         gic->ipi.send_self();
> -       check_acked("IPI: self", &mask);
> -       report_prefix_pop();
> +       report("self", check_acked(&mask));
>  }
>  
>  static void ipi_test_smp(void)
> @@ -203,7 +200,7 @@ static void ipi_test_smp(void)
>         for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>                 cpumask_clear_cpu(i, &mask);
>         gic_ipi_send_mask(IPI_IRQ, &mask);
> -       check_acked("IPI: directed", &mask);
> +       report("directed", check_acked(&mask));
>         report_prefix_pop();

Shouldn't we also drop the "target-list" prefix push/pop?

>  
>         report_prefix_push("broadcast");
> @@ -211,7 +208,7 @@ static void ipi_test_smp(void)
>         cpumask_copy(&mask, &cpu_present_mask);
>         cpumask_clear_cpu(smp_processor_id(), &mask);
>         gic->ipi.send_broadcast();
> -       check_acked("IPI: broadcast", &mask);
> +       report("broadcast", check_acked(&mask));
>         report_prefix_pop();
>  }

Shouldn't we also drop the "broadcast" prefix push/pop?

>  
> I've removed "IPI" from the report string because the prefixed was already pushed
> in main.
> 
> Andrew, what do you think? Are we missing something obvious? Do you have a better
> idea?

I'm happy to see cleanups and haven't had a chance to look too closely at
the gic tests in a while so I have no better ideas :-)

Thanks,
drew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 12/17] arm: gic: Change gic_read_iar() to take group parameter

[Andrew Jones]

On Tue, Nov 12, 2019 at 05:19:36PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 11/8/19 2:42 PM, Andre Przywara wrote:
> > Acknowledging a GIC group 0 interrupt requires us to use a different
> > system register on GICv3. To allow us to differentiate the two groups
> > later, add a group parameter to gic_read_iar(). For GICv2 we can use the
> > same CPU interface register to acknowledge group 0 as well, so we ignore
> > the parameter here.
> >
> > For now this is still using group 1 on every caller.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arm/gic.c                  |  4 ++--
> >  arm/micro-bench.c          |  2 +-
> >  arm/pl031.c                |  2 +-
> >  arm/timer.c                |  2 +-
> >  lib/arm/asm/arch_gicv3.h   | 11 +++++++++--
> >  lib/arm/asm/gic-v2.h       |  2 +-
> >  lib/arm/asm/gic-v3.h       |  2 +-
> >  lib/arm/asm/gic.h          |  2 +-
> >  lib/arm/gic-v2.c           |  3 ++-
> >  lib/arm/gic.c              |  6 +++---
> >  lib/arm64/asm/arch_gicv3.h | 10 ++++++++--
> >  11 files changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/arm/gic.c b/arm/gic.c
> > index a0511e5..7be13a6 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -156,7 +156,7 @@ static void check_irqnr(u32 irqnr, int expected)
> >  
> >  static void irq_handler(struct pt_regs *regs __unused)
> >  {
> > -	u32 irqstat = gic_read_iar();
> > +	u32 irqstat = gic_read_iar(1);
> >  	u32 irqnr = gic_iar_irqnr(irqstat);
> >  
> >  	if (irqnr == GICC_INT_SPURIOUS) {
> > @@ -288,7 +288,7 @@ static struct gic gicv3 = {
> >  
> >  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> >  {
> > -	u32 irqstat = gic_read_iar();
> > +	u32 irqstat = gic_read_iar(1);
> >  	u32 irqnr = gic_iar_irqnr(irqstat);
> >  
> >  	if (irqnr != GICC_INT_SPURIOUS) {
> > diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> > index 4612f41..2bfee68 100644
> > --- a/arm/micro-bench.c
> > +++ b/arm/micro-bench.c
> > @@ -33,7 +33,7 @@ static void ipi_irq_handler(struct pt_regs *regs)
> >  {
> >  	ipi_ready = false;
> >  	ipi_received = true;
> > -	gic_write_eoir(gic_read_iar());
> > +	gic_write_eoir(gic_read_iar(1));
> >  	ipi_ready = true;
> >  }
> >  
> > diff --git a/arm/pl031.c b/arm/pl031.c
> > index 5672f36..5be3d76 100644
> > --- a/arm/pl031.c
> > +++ b/arm/pl031.c
> > @@ -134,7 +134,7 @@ static void gic_irq_unmask(void)
> >  
> >  static void irq_handler(struct pt_regs *regs)
> >  {
> > -	u32 irqstat = gic_read_iar();
> > +	u32 irqstat = gic_read_iar(1);
> >  	u32 irqnr = gic_iar_irqnr(irqstat);
> >  
> >  	gic_write_eoir(irqstat);
> > diff --git a/arm/timer.c b/arm/timer.c
> > index 0b808d5..e5cc3b4 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -150,7 +150,7 @@ static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
> >  static void irq_handler(struct pt_regs *regs)
> >  {
> >  	struct timer_info *info;
> > -	u32 irqstat = gic_read_iar();
> > +	u32 irqstat = gic_read_iar(1);
> >  	u32 irqnr = gic_iar_irqnr(irqstat);
> >  
> >  	if (irqnr != GICC_INT_SPURIOUS)
> > diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
> > index 45b6096..52e7bba 100644
> > --- a/lib/arm/asm/arch_gicv3.h
> > +++ b/lib/arm/asm/arch_gicv3.h
> > @@ -16,6 +16,7 @@
> >  
> >  #define ICC_PMR				__ACCESS_CP15(c4, 0, c6, 0)
> >  #define ICC_SGI1R			__ACCESS_CP15_64(0, c12)
> > +#define ICC_IAR0			__ACCESS_CP15(c12, 0,  c8, 0)
> >  #define ICC_IAR1			__ACCESS_CP15(c12, 0, c12, 0)
> >  #define ICC_EOIR1			__ACCESS_CP15(c12, 0, c12, 1)
> >  #define ICC_IGRPEN1			__ACCESS_CP15(c12, 0, c12, 7)
> > @@ -30,9 +31,15 @@ static inline void gicv3_write_sgi1r(u64 val)
> >  	write_sysreg(val, ICC_SGI1R);
> >  }
> >  
> > -static inline u32 gicv3_read_iar(void)
> > +static inline u32 gicv3_read_iar(int group)
> >  {
> > -	u32 irqstat = read_sysreg(ICC_IAR1);
> > +	u32 irqstat;
> > +
> > +	if (group == 0)
> > +		irqstat = read_sysreg(ICC_IAR0);
> > +	else
> > +		irqstat = read_sysreg(ICC_IAR1);
> > +
> >  	dsb(sy);
> >  	return irqstat;
> >  }
> > diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> > index 1fcfd43..d50c610 100644
> > --- a/lib/arm/asm/gic-v2.h
> > +++ b/lib/arm/asm/gic-v2.h
> > @@ -32,7 +32,7 @@ extern struct gicv2_data gicv2_data;
> >  
> >  extern int gicv2_init(void);
> >  extern void gicv2_enable_defaults(void);
> > -extern u32 gicv2_read_iar(void);
> > +extern u32 gicv2_read_iar(int group);
> >  extern u32 gicv2_iar_irqnr(u32 iar);
> >  extern void gicv2_write_eoir(u32 irqstat);
> >  extern void gicv2_ipi_send_single(int irq, int cpu);
> > diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> > index 0a29610..ca19110 100644
> > --- a/lib/arm/asm/gic-v3.h
> > +++ b/lib/arm/asm/gic-v3.h
> > @@ -69,7 +69,7 @@ extern struct gicv3_data gicv3_data;
> >  
> >  extern int gicv3_init(void);
> >  extern void gicv3_enable_defaults(void);
> > -extern u32 gicv3_read_iar(void);
> > +extern u32 gicv3_read_iar(int group);
> >  extern u32 gicv3_iar_irqnr(u32 iar);
> >  extern void gicv3_write_eoir(u32 irqstat);
> >  extern void gicv3_ipi_send_single(int irq, int cpu);
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index 21cdb58..09663e7 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -68,7 +68,7 @@ extern void gic_enable_defaults(void);
> >   * below will work with any supported gic version.
> >   */
> >  extern int gic_version(void);
> > -extern u32 gic_read_iar(void);
> > +extern u32 gic_read_iar(int group);
> >  extern u32 gic_iar_irqnr(u32 iar);
> >  extern void gic_write_eoir(u32 irqstat);
> >  extern void gic_ipi_send_single(int irq, int cpu);
> > diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
> > index dc6a97c..b60967e 100644
> > --- a/lib/arm/gic-v2.c
> > +++ b/lib/arm/gic-v2.c
> > @@ -26,8 +26,9 @@ void gicv2_enable_defaults(void)
> >  	writel(GICC_ENABLE, cpu_base + GICC_CTLR);
> >  }
> >  
> > -u32 gicv2_read_iar(void)
> > +u32 gicv2_read_iar(int group)
> >  {
> > +	/* GICv2 acks both group0 and group1 IRQs with the same register. */
> >  	return readl(gicv2_cpu_base() + GICC_IAR);
> >  }
> >  
> > diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> > index cf4e811..b51eff5 100644
> > --- a/lib/arm/gic.c
> > +++ b/lib/arm/gic.c
> > @@ -12,7 +12,7 @@ struct gicv3_data gicv3_data;
> >  
> >  struct gic_common_ops {
> >  	void (*enable_defaults)(void);
> > -	u32 (*read_iar)(void);
> > +	u32 (*read_iar)(int group);
> >  	u32 (*iar_irqnr)(u32 iar);
> >  	void (*write_eoir)(u32 irqstat);
> >  	void (*ipi_send_single)(int irq, int cpu);
> > @@ -117,10 +117,10 @@ void gic_enable_defaults(void)
> >  	gic_common_ops->enable_defaults();
> >  }
> >  
> > -u32 gic_read_iar(void)
> > +u32 gic_read_iar(int group)
> >  {
> >  	assert(gic_common_ops && gic_common_ops->read_iar);
> > -	return gic_common_ops->read_iar();
> > +	return gic_common_ops->read_iar(group);
> >  }
> >  
> >  u32 gic_iar_irqnr(u32 iar)
> > diff --git a/lib/arm64/asm/arch_gicv3.h b/lib/arm64/asm/arch_gicv3.h
> > index a7994ec..876e1fc 100644
> > --- a/lib/arm64/asm/arch_gicv3.h
> > +++ b/lib/arm64/asm/arch_gicv3.h
> > @@ -11,6 +11,7 @@
> >  #include <asm/sysreg.h>
> >  
> >  #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
> > +#define ICC_IAR0_EL1			sys_reg(3, 0, 12, 8, 0)
> >  #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
> >  #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
> >  #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
> > @@ -38,10 +39,15 @@ static inline void gicv3_write_sgi1r(u64 val)
> >  	asm volatile("msr_s " xstr(ICC_SGI1R_EL1) ", %0" : : "r" (val));
> >  }
> >  
> > -static inline u32 gicv3_read_iar(void)
> > +static inline u32 gicv3_read_iar(int group)
> >  {
> >  	u64 irqstat;
> > -	asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
> > +
> > +	if (group == 0)
> > +		asm volatile("mrs_s %0, " xstr(ICC_IAR0_EL1) : "=r" (irqstat));
> > +	else
> > +		asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
> > +
> >  	dsb(sy);
> >  	return (u64)irqstat;
> >  }
> 
> I'm not sure this is the best approach. Now every test that happens to use the gic
> has to know about interrupt groups. Have you considered implementing the functions
> that you need for the test in arm/gic.c? Andrew, what do you think?
> 

I agree that we should provide a simple API, hiding as many GIC details as
possible, for tests that need to handle IRQs but don't need to know the
specifics. Indeed I think gic_read_iar() and gic_write_eoir() should be
renamed to something like gic_get_irqstat() and gic_eoi() to be more
generic. Also gic_iar_irqnr() should probably be reimplemented as per-gic
macros named something like GIC_IRQNR(irqstat). For groups we can change
the gic_get_irqstat(void) gic op to gic_get_group_irqstat(int group), and
then add this common gic function

u32 gic_get_irqstat(void)
{
  return gic_get_group_irqstat(GIC_DEFAULT_GROUP);
}

How's that sound?

Thanks,
drew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Vladimir Murzin]

Hi,

On 11/12/19 4:42 PM, Alexandru Elisei wrote:
> Are we not testing KVM? Why are we not treating a behaviour different than what
> KVM should emulate as a fail?

Can kvm-unit-tests be run with qemu TCG?

Cheers
Vladimir

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Andre Przywara]

On Thu, 14 Nov 2019 13:39:33 +0000
Vladimir Murzin <vladimir.murzin@arm.com> wrote:

> Hi,
> 
> On 11/12/19 4:42 PM, Alexandru Elisei wrote:
> > Are we not testing KVM? Why are we not treating a behaviour different than what
> > KVM should emulate as a fail?
> 
> Can kvm-unit-tests be run with qemu TCG?

Yes, it does that actually by default if you cross compile. I also tested this explicitly on TCG: unlike KVM that actually passes all those tests.
If you set the environment variable ACCEL to either tcg or kvm, you can select this at runtime:
$ ACCEL=tcg arm/run arm/gic.flat -smp 3 -append irq

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Vladimir Murzin]

On 11/14/19 2:17 PM, Andre Przywara wrote:
> On Thu, 14 Nov 2019 13:39:33 +0000
> Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> 
>> Hi,
>>
>> On 11/12/19 4:42 PM, Alexandru Elisei wrote:
>>> Are we not testing KVM? Why are we not treating a behaviour different than what
>>> KVM should emulate as a fail?
>>
>> Can kvm-unit-tests be run with qemu TCG?
> 
> Yes, it does that actually by default if you cross compile. I also tested this explicitly on TCG: unlike KVM that actually passes all those tests.
> If you set the environment variable ACCEL to either tcg or kvm, you can select this at runtime:
> $ ACCEL=tcg arm/run arm/gic.flat -smp 3 -append irq

Great! Then, IMO, it is absolutely valid to test this functionality!

Thanks
Vladimir

> 
> Cheers,
> Andre
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Alexandru Elisei]

Hi,

On 11/14/19 2:50 PM, Vladimir Murzin wrote:
> On 11/14/19 2:17 PM, Andre Przywara wrote:
>> On Thu, 14 Nov 2019 13:39:33 +0000
>> Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>
>>> Hi,
>>>
>>> On 11/12/19 4:42 PM, Alexandru Elisei wrote:
>>>> Are we not testing KVM? Why are we not treating a behaviour different than what
>>>> KVM should emulate as a fail?
>>> Can kvm-unit-tests be run with qemu TCG?
>> Yes, it does that actually by default if you cross compile. I also tested this explicitly on TCG: unlike KVM that actually passes all those tests.
>> If you set the environment variable ACCEL to either tcg or kvm, you can select this at runtime:
>> $ ACCEL=tcg arm/run arm/gic.flat -smp 3 -append irq
> Great! Then, IMO, it is absolutely valid to test this functionality!

TCG emulates a GIC with a single security state for me:

/usr/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=3,accel=tcg
-cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd
-chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel
arm/gic.flat -append irq
PASS: gicv3: irq: SPI triggered by CPU write
PASS: gicv3: irq: disabled SPI does not fire
PASS: gicv3: irq: now enabled SPI fires
INFO: gicv3: irq: GROUP: GIC is one security state only
[..]

But that could change someday, so I'm fine with failing only if we are not allowed
to have GICD_CTLR.DS=1, because that will prevent us from testing group 0 interrupts.

Thanks,
Alex
> Thanks
> Vladimir
>
>> Cheers,
>> Andre
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Peter Maydell]

On Thu, 14 Nov 2019 at 15:21, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> TCG emulates a GIC with a single security state for me:
>
> /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=3,accel=tcg
> -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd
> -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel
> arm/gic.flat -append irq

The virt board doesn't do EL3 by default, but if you add -machine secure=true
to your command line then it it should emulate it, including a
trustzone-aware GIC.

thanks
-- PMM

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Alexandru Elisei]

Hi,

On 11/14/19 3:27 PM, Peter Maydell wrote:
> On Thu, 14 Nov 2019 at 15:21, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> TCG emulates a GIC with a single security state for me:
>>
>> /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=3,accel=tcg
>> -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd
>> -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel
>> arm/gic.flat -append irq
> The virt board doesn't do EL3 by default, but if you add -machine secure=true
> to your command line then it it should emulate it, including a
> trustzone-aware GIC.
>
> thanks
> -- PMM

Indeed, and that made the test fail because apparently qemu implements it as
RAZ/WI (which is allowed by the architecture). Thank you for the suggestion!

Thanks,
Alex



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS

[Peter Maydell]

On Thu, 14 Nov 2019 at 15:47, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> On 11/14/19 3:27 PM, Peter Maydell wrote:
> > The virt board doesn't do EL3 by default, but if you add -machine secure=true
> > to your command line then it it should emulate it, including a
> > trustzone-aware GIC.

> Indeed, and that made the test fail because apparently qemu implements it as
> RAZ/WI (which is allowed by the architecture). Thank you for the suggestion!

Hmm. The behaviour QEMU thinks it's implementing is:

 * if we have only one security state, then CTLR.DS is RAO/WI
 * if we have two security states, then:
    - for access from NonSecure, CTLR.DS is RAZ/WI
    - for access from Secure, CTLR.DS is initially 0, and is
      writeable, but if you write 1 to it then the only way
      to get it to go back to zero is to reset the system

thanks
-- PMM

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call

[Alexandru Elisei]

Hi,

On 11/14/19 12:32 PM, Andrew Jones wrote:
> On Tue, Nov 12, 2019 at 03:23:04PM +0000, Alexandru Elisei wrote:
>> check_acked is starting to become hard to read.
> Agreed. check_acked() could probably have some of its subtests factored
> out to improve its readability.
>
>> The function itself is rather inconsistent, as it mixes regular
>> printf's with report_info's.
> Sounds good
>
>> The return value is also never used:
>>
>> $ awk '/check_acked\(/ && !/const/' arm/gic.c
>>     check_acked("IPI: self", &mask);
>>     check_acked("IPI: directed", &mask);
>>     check_acked("IPI: broadcast", &mask);
> That's good, since it's a void function :-)

Sorry, got confused, this patch changes it to return a value, and that value is
ignored in the existing functions (the ones I listed above), which would make the
usage of check_acked very inconsistent.

>> What I'm thinking is that we can rewrite check_acked to return true/false (or
>> 0/1), meaning success or failure, remove the testname parameter, replace the
>> printfs to report_info, and have the caller do a report based on the value
>> returned by check_acked.
>>
>> Rough version, compile tested only, I'm sure it can be improved:
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index adb6aa464513..5453f2fd3d5f 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -60,11 +60,11 @@ static void stats_reset(void)
>>         smp_wmb();
>>  }
>>  
>> -static void check_acked(const char *testname, cpumask_t *mask)
>> +static bool check_acked(cpumask_t *mask)
> We have several check_* functions in arm/gic.c, and they're all void
> functions. Changing this one to a bool would be inconsistent, but
> maybe that consistency isn't that important, or maybe they should all
> be bool?

I think they should stay void, because they compute statistics, or print a warning
if we got a spurious interrupt (check_spurious). I'm not really sure what to do
about check_acked at the moment, I'll think about it some more.

>>  {
>>         int missing = 0, extra = 0, unexpected = 0;
>>         int nr_pass, cpu, i;
>> -       bool bad = false;
>> +       bool success = true;
>>  
>>         /* Wait up to 5s for all interrupts to be delivered */
>>         for (i = 0; i < 50; ++i) {
>> @@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>                                 acked[cpu] == 1 : acked[cpu] == 0;
>>  
>>                         if (bad_sender[cpu] != -1) {
>> -                               printf("cpu%d received IPI from wrong sender %d\n",
>> +                               report_info("cpu%d received IPI from wrong sender
>> %d\n",
>>                                         cpu, bad_sender[cpu]);
>> -                               bad = true;
>> +                               success = false;
>>                         }
>>  
>>                         if (bad_irq[cpu] != -1) {
>> -                               printf("cpu%d received wrong irq %d\n",
>> +                               report_info("cpu%d received wrong irq %d\n",
>>                                         cpu, bad_irq[cpu]);
>> -                               bad = true;
>> +                               success = false;
>>                         }
>>                 }
>>                 if (nr_pass == nr_cpus) {
>> -                       report("%s", !bad, testname);
>>                         if (i)
>>                                 report_info("took more than %d ms", i * 100);
>> -                       return;
>> +                       return success;
>>                 }
>>         }
>>  
>> @@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>                 }
>>         }
>>  
>> -       report("%s", false, testname);
>>         report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>>                     missing, extra, unexpected);
>> +       return false;
>>  }
>>  
>>  static void check_spurious(void)
>> @@ -183,13 +182,11 @@ static void ipi_test_self(void)
>>  {
>>         cpumask_t mask;
>>  
>> -       report_prefix_push("self");
>>         stats_reset();
>>         cpumask_clear(&mask);
>>         cpumask_set_cpu(smp_processor_id(), &mask);
>>         gic->ipi.send_self();
>> -       check_acked("IPI: self", &mask);
>> -       report_prefix_pop();
>> +       report("self", check_acked(&mask));
>>  }
>>  
>>  static void ipi_test_smp(void)
>> @@ -203,7 +200,7 @@ static void ipi_test_smp(void)
>>         for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>>                 cpumask_clear_cpu(i, &mask);
>>         gic_ipi_send_mask(IPI_IRQ, &mask);
>> -       check_acked("IPI: directed", &mask);
>> +       report("directed", check_acked(&mask));
>>         report_prefix_pop();
> Shouldn't we also drop the "target-list" prefix push/pop?
>
>>  
>>         report_prefix_push("broadcast");
>> @@ -211,7 +208,7 @@ static void ipi_test_smp(void)
>>         cpumask_copy(&mask, &cpu_present_mask);
>>         cpumask_clear_cpu(smp_processor_id(), &mask);
>>         gic->ipi.send_broadcast();
>> -       check_acked("IPI: broadcast", &mask);
>> +       report("broadcast", check_acked(&mask));
>>         report_prefix_pop();
>>  }
> Shouldn't we also drop the "broadcast" prefix push/pop?

My suggestion was a quick hack to give an idea of how it might look, it can
definitely be improved :)

Thanks,
Alex
>>  
>> I've removed "IPI" from the report string because the prefixed was already pushed
>> in main.
>>
>> Andrew, what do you think? Are we missing something obvious? Do you have a better
>> idea?
> I'm happy to see cleanups and haven't had a chance to look too closely at
> the gic tests in a while so I have no better ideas :-)
>
> Thanks,
> drew
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel