[Qemu-devel] [kvm-unit-tests PATCH 0/4] kvm-unit-tests: add first GIC MMIO tests

[Qemu-devel] [kvm-unit-tests PATCH 0/4] kvm-unit-tests: add first GIC MMIO tests

[Andre Przywara]

The GIC spec mandates certain constraints on how to acccess the MMIO
mapped registers, both in terms of which registers are available and also
in terms of which bits within a register should be masked, for instance.
Since we went through some lengths in the KVM emulation to implement this,
it's about time to give this actually a test beyond what the kernel as a
GIC user actually implements - for instance we ignore priorities in Linux.

This series tries to attack some constraints, on a low-hanging-fruit base.
It focusses on some generic registers and the PRIORITY and TARGETS registers
of GICv2. GICv3 is only covered to the point where the GICv2 tests overlap
with GICv3.

This actually revealed genuine bugs in the KVM emulation, for which I sent
fixes in the last days. With those fixes the kernel passes all tests.

Please have a look whether this makes sense and should eventually be
extended on that base.

Applies on top of master + Drew's v6 series.

Cheers,
Andre.

Andre Przywara (4):
  arm/arm64: GIC: basic GICv2 MMIO tests
  arm/arm64: GICv2: add GICD_IPRIORITYR testing
  arm/arm64: GICv2: add GICD_ITARGETSR testing
  arm/arm64: GICv3: add TYPER test

 arm/gic.c         | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |  12 +++
 lib/arm/asm/gic.h |   3 +
 3 files changed, 268 insertions(+)

-- 
2.9.0

[Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Andre Przywara]

This adds an MMIO subtest to the GIC test.
It accesses some generic GICv2 registers and does some sanity tests,
like checking for some of them being read-only.

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

diff --git a/arm/gic.c b/arm/gic.c
index 638b8b1..ba2585b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -3,6 +3,7 @@
  *
  * GICv2
  *   + test sending/receiving IPIs
+ *   + MMIO access tests
  * GICv3
  *   + test sending/receiving IPIs
  *
@@ -274,6 +275,98 @@ static struct gic gicv3 = {
 	},
 };
 
+static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
+{
+	u32 reg;
+
+	writel(pattern, address);
+	reg = readl(address);
+
+	if (reg != orig)
+		writel(orig, address);
+
+	return reg == orig;
+}
+
+static bool test_readonly_32(void *address, bool razwi)
+{
+	u32 orig, pattern;
+
+	orig = readl(address);
+	if (razwi && orig)
+		return false;
+
+	pattern = 0xffffffff;
+	if (orig != pattern) {
+		if (!test_ro_pattern_32(address, pattern, orig))
+			return false;
+	}
+
+	pattern = 0xa5a55a5a;
+	if (orig != pattern) {
+		if (!test_ro_pattern_32(address, pattern, orig))
+			return false;
+	}
+
+	pattern = 0;
+	if (orig != pattern) {
+		if (!test_ro_pattern_32(address, pattern, orig))
+			return false;
+	}
+
+	return true;
+}
+
+static bool test_typer_v2(uint32_t reg)
+{
+	int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
+
+	report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
+	       nr_gic_cpus);
+
+	return true;
+}
+
+static int gic_test_mmio(int gic_version)
+{
+	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 + 0xfe8;
+		break;
+	case 0x3:
+		report_abort("GICv3 MMIO tests NYI");
+		return -1;
+	default:
+		report_abort("GIC version %d not supported", gic_version);
+		return 0;
+	}
+
+	reg = readl(gic_dist_base + GICD_TYPER);
+	nr_irqs = 32 * ((reg & 0x1f) + 1);
+	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
+
+	test_typer_v2(reg);
+
+	report("IIDR: 0x%x", 1, 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 (0x%x)",
+	       test_readonly_32(idreg, false),
+	       reg);
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	char pfx[8];
@@ -332,6 +425,12 @@ int main(int argc, char **argv)
 		}
 		ipi_test();
 
+	} else if (!strcmp(argv[1], "mmio")) {
+		report_prefix_push(argv[1]);
+
+		gic_test_mmio(gic_version());
+
+		report_prefix_pop();
 	} else {
 		report_abort("Unknown subtest '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index c7392c7..0162e5a 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
 extra_params = -machine gic-version=2 -append 'ipi'
 groups = gic
 
+[gicv2-mmio]
+file = gic.flat
+smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
+extra_params = -machine gic-version=2 -append 'mmio'
+groups = gic
+
 [gicv3-ipi]
 file = gic.flat
 smp = $MAX_SMP
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index c2267b6..cef748d 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -10,10 +10,12 @@
 /* Distributor registers */
 #define GICD_CTLR			0x0000
 #define GICD_TYPER			0x0004
+#define GICD_IIDR			0x0008
 #define GICD_IGROUPR			0x0080
 #define GICD_ISENABLER			0x0100
 #define GICD_IPRIORITYR			0x0400
 #define GICD_SGIR			0x0f00
+#define GICD_ICPIDR2			0x0fe8
 
 #define GICD_TYPER_IRQS(typer)		((((typer) & 0x1f) + 1) * 32)
 #define GICD_INT_EN_SET_SGI		0x0000ffff
-- 
2.9.0

[Qemu-devel] [kvm-unit-tests PATCH 2/4] arm/arm64: GICv2: add GICD_IPRIORITYR testing

[Andre Przywara]

Some tests for the IPRIORITY registers. The significant number of bits
is IMPLEMENTATION DEFINED, but should be the same for every IRQ.
Also these registers must be byte-accessible.
Check that accesses beyond the implemented IRQ limit are actually
read-as-zero/write-ignore.

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

diff --git a/arm/gic.c b/arm/gic.c
index ba2585b..a27da2c 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -327,6 +327,76 @@ static bool test_typer_v2(uint32_t reg)
 	return true;
 }
 
+#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
+#define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
+					((new) << ((byte) * 8)))
+
+static bool test_priorities(int nr_irqs, void *priptr)
+{
+	u32 orig_prio, reg, pri_bits;
+	u32 pri_mask, pattern;
+
+	orig_prio = readl(priptr + 32);
+	report_prefix_push("IPRIORITYR");
+
+	/*
+	 * Determine implemented number of priority bits by writing all 1's
+	 * and checking the number of cleared bits in the value read back.
+	 */
+	writel(0xffffffff, priptr + 32);
+	pri_mask = readl(priptr + 32);
+
+	reg = ~pri_mask;
+	report("consistent priority masking (0x%08x)",
+	       (((reg >> 16) == (reg & 0xffff)) &&
+	        ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
+
+	reg = reg & 0xff;
+	for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
+		;
+	report("implements at least 4 priority bits (%d)",
+	       pri_bits >= 4, pri_bits);
+
+	pattern = 0;
+	writel(pattern, priptr + 32);
+	report("clearing priorities", readl(priptr + 32) == pattern);
+
+	pattern = 0xffffffff;
+	writel(pattern, priptr + 32);
+	report("filling priorities",
+	       readl(priptr + 32) == (pattern & pri_mask));
+
+	report("accesses beyond limit RAZ/WI",
+	       test_readonly_32(priptr + nr_irqs, true));
+
+	writel(pattern, priptr + nr_irqs - 4);
+	report("accessing last SPIs",
+	       readl(priptr + nr_irqs - 4) == (pattern & pri_mask));
+
+	pattern = 0xff7fbf3f;
+	writel(pattern, priptr + 32);
+	report("priorities are preserved",
+	       readl(priptr + 32) == (pattern & pri_mask));
+
+	/*
+	 * The PRIORITY registers are byte accessible, do a byte-wide
+	 * read and write of known content to check for this.
+	 */
+	reg = readb(priptr + 33);
+	report("byte reads successful (0x%08x => 0x%02x)",
+	       reg == (BYTE(pattern, 1) & pri_mask), pattern & pri_mask, reg);
+
+	pattern = REPLACE_BYTE(pattern, 2, 0x1f);
+	writeb(BYTE(pattern, 2), priptr + 34);
+	reg = readl(priptr + 32);
+	report("byte writes successful (0x%02x => 0x%08x)",
+	       reg == (pattern & pri_mask), BYTE(pattern, 2) & pri_mask, reg);
+
+	report_prefix_pop();
+	writel(orig_prio, priptr + 32);
+	return true;
+}
+
 static int gic_test_mmio(int gic_version)
 {
 	u32 reg;
@@ -364,6 +434,8 @@ static int gic_test_mmio(int gic_version)
 	       test_readonly_32(idreg, false),
 	       reg);
 
+	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
+
 	return 0;
 }
 
-- 
2.9.0

[Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Andre Przywara]

Some tests for the ITARGETS registers.
Bits corresponding to non-existent CPUs must be RAZ/WI.
These registers must be byte-accessible, also check that accesses beyond
the implemented IRQ limit are actually read-as-zero/write-ignore.

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

diff --git a/arm/gic.c b/arm/gic.c
index a27da2c..02b1be1 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
 	return true;
 }
 
+static bool test_targets(int nr_irqs)
+{
+	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
+	u32 orig_targets;
+	u32 cpu_mask;
+	u32 pattern, reg;
+
+	orig_targets = readl(targetsptr + 32);
+	report_prefix_push("ITARGETSR");
+
+	cpu_mask = (1 << nr_cpus) - 1;
+	cpu_mask |= cpu_mask << 8;
+	cpu_mask |= cpu_mask << 16;
+
+	/* Check that bits for non implemented CPUs are RAZ/WI. */
+	if (nr_cpus < 8) {
+		writel(0xffffffff, targetsptr + 32);
+		report("bits for %d non-existent CPUs masked",
+		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
+	} else {
+		report_skip("CPU masking (all CPUs implemented)");
+	}
+
+	report("accesses beyond limit RAZ/WI",
+	       test_readonly_32(targetsptr + nr_irqs, true));
+
+	pattern = 0x0103020f;
+	writel(pattern, targetsptr + 32);
+	reg = readl(targetsptr + 32);
+	report("register content preserved (%08x => %08x)",
+	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
+
+	/*
+	 * The TARGETS registers are byte accessible, do a byte-wide
+	 * read and write of known content to check for this.
+	 */
+	reg = readb(targetsptr + 33);
+	report("byte reads successful (0x%08x => 0x%02x)",
+	       reg == (BYTE(pattern, 1) & cpu_mask),
+	       pattern & cpu_mask, reg);
+
+	pattern = REPLACE_BYTE(pattern, 2, 0x04);
+	writeb(BYTE(pattern, 2), targetsptr + 34);
+	reg = readl(targetsptr + 32);
+	report("byte writes successful (0x%02x => 0x%08x)",
+	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);
+
+	writel(orig_targets, targetsptr + 32);
+	return true;
+}
+
 static int gic_test_mmio(int gic_version)
 {
 	u32 reg;
@@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
 
 	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
 
+	if (gic_version == 2)
+		test_targets(nr_irqs);
+
 	return 0;
 }
 
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index cef748d..6f170cb 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -14,6 +14,7 @@
 #define GICD_IGROUPR			0x0080
 #define GICD_ISENABLER			0x0100
 #define GICD_IPRIORITYR			0x0400
+#define GICD_ITARGETSR			0x0800
 #define GICD_SGIR			0x0f00
 #define GICD_ICPIDR2			0x0fe8
 
-- 
2.9.0

[Qemu-devel] [kvm-unit-tests PATCH 4/4] arm/arm64: GICv3: add TYPER test

[Andre Przywara]

Add a simple test for the GICv3 TYPER test, which does only one basic
check to ensure we have actually enough interrupt IDs if we support
LPIs.
Allow a GICv3 guest to do the common MMIO checks as well, where the
register semantics are shared with a GICv2.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c         | 34 +++++++++++++++++++++++++++++++---
 arm/unittests.cfg |  6 ++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 02b1be1..7de0e47 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -327,6 +327,30 @@ static bool test_typer_v2(uint32_t reg)
 	return true;
 }
 
+static bool test_typer_v3(uint32_t reg)
+{
+	int nr_intids;
+
+	report("GIC emulation %ssupport%s MBIs", 1,
+	       reg & BIT(16) ? "" : "does not ",
+	       reg & BIT(16) ? "s" : "");
+	report("GIC emulation %ssupport%s LPIs", 1,
+	       reg & BIT(17) ? "" : "does not ",
+	       reg & BIT(17) ? "s" : "");
+	report("GIC emulation %ssupport%s Aff3", 1,
+	       reg & BIT(24) ? "" : "does not ",
+	       reg & BIT(24) ? "s" : "");
+
+	nr_intids = BIT(((reg >> 19) & 0x1f) + 1);
+	report("%d interrupt IDs implemented", 1, nr_intids);
+
+	if (reg & BIT(17))
+		report("%d LPIs supported", nr_intids > 8192,
+		       nr_intids > 8192 ? nr_intids - 8192 : 0);
+
+	return true;
+}
+
 #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
 #define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
 					((new) << ((byte) * 8)))
@@ -460,8 +484,9 @@ static int gic_test_mmio(int gic_version)
 		idreg = gic_dist_base + 0xfe8;
 		break;
 	case 0x3:
-		report_abort("GICv3 MMIO tests NYI");
-		return -1;
+		gic_dist_base = gicv3_dist_base();
+		idreg = gic_dist_base + 0xffe8;
+		break;
 	default:
 		report_abort("GIC version %d not supported", gic_version);
 		return 0;
@@ -471,7 +496,10 @@ static int gic_test_mmio(int gic_version)
 	nr_irqs = 32 * ((reg & 0x1f) + 1);
 	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
 
-	test_typer_v2(reg);
+	if (gic_version == 2)
+		test_typer_v2(reg);
+	else
+		test_typer_v3(reg);
 
 	report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
 
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 0162e5a..b432346 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -78,3 +78,9 @@ file = gic.flat
 smp = $MAX_SMP
 extra_params = -machine gic-version=3 -append 'ipi'
 groups = gic
+
+[gicv3-mmio]
+file = gic.flat
+smp = $MAX_SMP
+extra_params = -machine gic-version=3 -append 'mmio'
+groups = gic
-- 
2.9.0

Re: [Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Andrew Jones]

Hi Andre,

I'm so pleased to see this series. Thank you!

On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote:
> This adds an MMIO subtest to the GIC test.
> It accesses some generic GICv2 registers and does some sanity tests,
> like checking for some of them being read-only.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  6 ++++
>  lib/arm/asm/gic.h |  2 ++
>  3 files changed, 107 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 638b8b1..ba2585b 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -3,6 +3,7 @@
>   *
>   * GICv2
>   *   + test sending/receiving IPIs
> + *   + MMIO access tests
>   * GICv3
>   *   + test sending/receiving IPIs
>   *
> @@ -274,6 +275,98 @@ static struct gic gicv3 = {
>  	},
>  };
>  
> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
> +{
> +	u32 reg;
> +
> +	writel(pattern, address);
> +	reg = readl(address);
> +
> +	if (reg != orig)
> +		writel(orig, address);
> +
> +	return reg == orig;
> +}
> +
> +static bool test_readonly_32(void *address, bool razwi)
> +{
> +	u32 orig, pattern;
> +
> +	orig = readl(address);
> +	if (razwi && orig)
> +		return false;
> +
> +	pattern = 0xffffffff;
> +	if (orig != pattern) {
> +		if (!test_ro_pattern_32(address, pattern, orig))
> +			return false;
> +	}
> +
> +	pattern = 0xa5a55a5a;
> +	if (orig != pattern) {
> +		if (!test_ro_pattern_32(address, pattern, orig))
> +			return false;
> +	}
> +
> +	pattern = 0;
> +	if (orig != pattern) {
> +		if (!test_ro_pattern_32(address, pattern, orig))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool test_typer_v2(uint32_t reg)
> +{
> +	int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
> +
> +	report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
> +	       nr_gic_cpus);
> +
> +	return true;

This test function can be a void.

> +}
> +
> +static int gic_test_mmio(int gic_version)
> +{
> +	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 + 0xfe8;

I see below you introduce GICD_ICPIDR2, so I guess you can use it here.

> +		break;
> +	case 0x3:
> +		report_abort("GICv3 MMIO tests NYI");
> +		return -1;

can't reach this return

> +	default:
> +		report_abort("GIC version %d not supported", gic_version);
> +		return 0;

can't reach this return

> +	}
> +
> +	reg = readl(gic_dist_base + GICD_TYPER);
> +	nr_irqs = 32 * ((reg & 0x1f) + 1);

Any reason to avoid using GICD_TYPER_IRQS() here?

> +	report("number of implemented SPIs: %d", 1, nr_irqs - 32);

We usually just use printf for informational output (but we should
probably add a 'report_info' in order to keep the prefixes. I can
do that now.) Anyway, please s/1/true

> +
> +	test_typer_v2(reg);
> +
> +	report("IIDR: 0x%x", 1, 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 (0x%x)",
> +	       test_readonly_32(idreg, false),
> +	       reg);
> +
> +	return 0;

You may want %08x for all your register printing.

Since you either abort or always return success, then this function can be
a void.

> +}
> +
>  int main(int argc, char **argv)
>  {
>  	char pfx[8];
> @@ -332,6 +425,12 @@ int main(int argc, char **argv)
>  		}
>  		ipi_test();
>  
> +	} else if (!strcmp(argv[1], "mmio")) {
> +		report_prefix_push(argv[1]);
> +
> +		gic_test_mmio(gic_version());

Any reason to pass gic_version() here instead of just using it
in gic_test_mmio?

> +
> +		report_prefix_pop();
>  	} else {
>  		report_abort("Unknown subtest '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index c7392c7..0162e5a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'ipi'
>  groups = gic
>  
> +[gicv2-mmio]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> +extra_params = -machine gic-version=2 -append 'mmio'
> +groups = gic
> +
>  [gicv3-ipi]
>  file = gic.flat
>  smp = $MAX_SMP
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index c2267b6..cef748d 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -10,10 +10,12 @@
>  /* Distributor registers */
>  #define GICD_CTLR			0x0000
>  #define GICD_TYPER			0x0004
> +#define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
>  #define GICD_IPRIORITYR			0x0400
>  #define GICD_SGIR			0x0f00
> +#define GICD_ICPIDR2			0x0fe8
>  
>  #define GICD_TYPER_IRQS(typer)		((((typer) & 0x1f) + 1) * 32)
>  #define GICD_INT_EN_SET_SGI		0x0000ffff
> -- 
> 2.9.0
> 
>

Thanks,
drew 

Re: [Qemu-devel] [kvm-unit-tests PATCH 2/4] arm/arm64: GICv2: add GICD_IPRIORITYR testing

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:50PM +0000, Andre Przywara wrote:
> Some tests for the IPRIORITY registers. The significant number of bits
> is IMPLEMENTATION DEFINED, but should be the same for every IRQ.
> Also these registers must be byte-accessible.
> Check that accesses beyond the implemented IRQ limit are actually
> read-as-zero/write-ignore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ba2585b..a27da2c 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -327,6 +327,76 @@ static bool test_typer_v2(uint32_t reg)
>  	return true;
>  }
>  
> +#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
> +#define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
> +					((new) << ((byte) * 8)))
> +
> +static bool test_priorities(int nr_irqs, void *priptr)
> +{
> +	u32 orig_prio, reg, pri_bits;
> +	u32 pri_mask, pattern;
> +
> +	orig_prio = readl(priptr + 32);
> +	report_prefix_push("IPRIORITYR");
> +
> +	/*
> +	 * Determine implemented number of priority bits by writing all 1's
> +	 * and checking the number of cleared bits in the value read back.
> +	 */
> +	writel(0xffffffff, priptr + 32);
> +	pri_mask = readl(priptr + 32);
> +
> +	reg = ~pri_mask;
> +	report("consistent priority masking (0x%08x)",
> +	       (((reg >> 16) == (reg & 0xffff)) &&
> +	        ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
> +
> +	reg = reg & 0xff;
> +	for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
> +		;
> +	report("implements at least 4 priority bits (%d)",
> +	       pri_bits >= 4, pri_bits);
> +
> +	pattern = 0;
> +	writel(pattern, priptr + 32);
> +	report("clearing priorities", readl(priptr + 32) == pattern);
> +
> +	pattern = 0xffffffff;
> +	writel(pattern, priptr + 32);
> +	report("filling priorities",
> +	       readl(priptr + 32) == (pattern & pri_mask));
> +
> +	report("accesses beyond limit RAZ/WI",
> +	       test_readonly_32(priptr + nr_irqs, true));
> +
> +	writel(pattern, priptr + nr_irqs - 4);
> +	report("accessing last SPIs",
> +	       readl(priptr + nr_irqs - 4) == (pattern & pri_mask));
> +
> +	pattern = 0xff7fbf3f;
> +	writel(pattern, priptr + 32);
> +	report("priorities are preserved",
> +	       readl(priptr + 32) == (pattern & pri_mask));
> +
> +	/*
> +	 * The PRIORITY registers are byte accessible, do a byte-wide
> +	 * read and write of known content to check for this.
> +	 */
> +	reg = readb(priptr + 33);
> +	report("byte reads successful (0x%08x => 0x%02x)",
> +	       reg == (BYTE(pattern, 1) & pri_mask), pattern & pri_mask, reg);
> +
> +	pattern = REPLACE_BYTE(pattern, 2, 0x1f);
> +	writeb(BYTE(pattern, 2), priptr + 34);
> +	reg = readl(priptr + 32);
> +	report("byte writes successful (0x%02x => 0x%08x)",
> +	       reg == (pattern & pri_mask), BYTE(pattern, 2) & pri_mask, reg);
> +
> +	report_prefix_pop();
> +	writel(orig_prio, priptr + 32);
> +	return true;

Might be nice to have FIRST_SPI and maybe LAST_SPI macros to avoid all
the +32's

This function always returns true, so it can be a void.

> +}
> +
>  static int gic_test_mmio(int gic_version)
>  {
>  	u32 reg;
> @@ -364,6 +434,8 @@ static int gic_test_mmio(int gic_version)
>  	       test_readonly_32(idreg, false),
>  	       reg);
>  
> +	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);

Feel free to add state like nr_irqs and dist_base to the gic struct
defined in arm/gic.c. That struct should provide the abstraction
needed to handle both gicv2 and gicv3 and contain anything that the
test functions need to refer to frequently. Using it should help
reduce the amount of parameters passed around.

> +
>  	return 0;
>  }
>  
> -- 
> 2.9.0

Otherwise looks good to me

Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Andre Przywara]

Hi Drew,

On 18/11/16 13:06, Andrew Jones wrote:
> Hi Andre,
> 
> I'm so pleased to see this series. Thank you!
> 
> On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote:
>> This adds an MMIO subtest to the GIC test.
>> It accesses some generic GICv2 registers and does some sanity tests,
>> like checking for some of them being read-only.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg |  6 ++++
>>  lib/arm/asm/gic.h |  2 ++
>>  3 files changed, 107 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 638b8b1..ba2585b 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -3,6 +3,7 @@
>>   *
>>   * GICv2
>>   *   + test sending/receiving IPIs
>> + *   + MMIO access tests
>>   * GICv3
>>   *   + test sending/receiving IPIs
>>   *
>> @@ -274,6 +275,98 @@ static struct gic gicv3 = {
>>  	},
>>  };
>>  
>> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
>> +{
>> +	u32 reg;
>> +
>> +	writel(pattern, address);
>> +	reg = readl(address);
>> +
>> +	if (reg != orig)
>> +		writel(orig, address);
>> +
>> +	return reg == orig;
>> +}
>> +
>> +static bool test_readonly_32(void *address, bool razwi)
>> +{
>> +	u32 orig, pattern;
>> +
>> +	orig = readl(address);
>> +	if (razwi && orig)
>> +		return false;
>> +
>> +	pattern = 0xffffffff;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	pattern = 0xa5a55a5a;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	pattern = 0;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static bool test_typer_v2(uint32_t reg)
>> +{
>> +	int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
>> +
>> +	report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
>> +	       nr_gic_cpus);
>> +
>> +	return true;
> 
> This test function can be a void.
> 
>> +}
>> +
>> +static int gic_test_mmio(int gic_version)
>> +{
>> +	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 + 0xfe8;
> 
> I see below you introduce GICD_ICPIDR2, so I guess you can use it here.
> 
>> +		break;
>> +	case 0x3:
>> +		report_abort("GICv3 MMIO tests NYI");
>> +		return -1;
> 
> can't reach this return

But we need to tell GCC about this, because otherwise we get all kind of
warnings (including bogus "maybe unused" warnings).
__attribute__ ((noreturn)) seems the way to go here, but this is
currently giving me a hard time ...

>> +	default:
>> +		report_abort("GIC version %d not supported", gic_version);
>> +		return 0;
> 
> can't reach this return
> 
>> +	}
>> +
>> +	reg = readl(gic_dist_base + GICD_TYPER);
>> +	nr_irqs = 32 * ((reg & 0x1f) + 1);
> 
> Any reason to avoid using GICD_TYPER_IRQS() here?

On the first write I wasn't aware of it, on a second thought then I
wanted to avoid using the macro copied from Linux.

But you are right, I will use it here.

> 
>> +	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
> 
> We usually just use printf for informational output (but we should
> probably add a 'report_info' in order to keep the prefixes. I can
> do that now.) Anyway, please s/1/true

I saw your patch, will use that.

>> +
>> +	test_typer_v2(reg);
>> +
>> +	report("IIDR: 0x%x", 1, 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 (0x%x)",
>> +	       test_readonly_32(idreg, false),
>> +	       reg);
>> +
>> +	return 0;
> 
> You may want %08x for all your register printing.
> 
> Since you either abort or always return success, then this function can be
> a void.
> 
>> +}
>> +
>>  int main(int argc, char **argv)
>>  {
>>  	char pfx[8];
>> @@ -332,6 +425,12 @@ int main(int argc, char **argv)
>>  		}
>>  		ipi_test();
>>  
>> +	} else if (!strcmp(argv[1], "mmio")) {
>> +		report_prefix_push(argv[1]);
>> +
>> +		gic_test_mmio(gic_version());
> 
> Any reason to pass gic_version() here instead of just using it
> in gic_test_mmio?

Not really, I originally wanted to pass this variable on in a clean
fashion to allow sharing tests.
But using the function shouldn't make any difference anymore, so I can
easily replace it.

"Yes, will fix" to all the other comments.

Thanks for having a look!

Cheers,
Andre.

>> +
>> +		report_prefix_pop();
>>  	} else {
>>  		report_abort("Unknown subtest '%s'", argv[1]);
>>  	}
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index c7392c7..0162e5a 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>>  extra_params = -machine gic-version=2 -append 'ipi'
>>  groups = gic
>>  
>> +[gicv2-mmio]
>> +file = gic.flat
>> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>> +extra_params = -machine gic-version=2 -append 'mmio'
>> +groups = gic
>> +
>>  [gicv3-ipi]
>>  file = gic.flat
>>  smp = $MAX_SMP
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index c2267b6..cef748d 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -10,10 +10,12 @@
>>  /* Distributor registers */
>>  #define GICD_CTLR			0x0000
>>  #define GICD_TYPER			0x0004
>> +#define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>>  #define GICD_IPRIORITYR			0x0400
>>  #define GICD_SGIR			0x0f00
>> +#define GICD_ICPIDR2			0x0fe8
>>  
>>  #define GICD_TYPER_IRQS(typer)		((((typer) & 0x1f) + 1) * 32)
>>  #define GICD_INT_EN_SET_SGI		0x0000ffff
>> -- 
>> 2.9.0
>>
>>
> 
> Thanks,
> drew 
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
> Some tests for the ITARGETS registers.
> Bits corresponding to non-existent CPUs must be RAZ/WI.
> These registers must be byte-accessible, also check that accesses beyond
> the implemented IRQ limit are actually read-as-zero/write-ignore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/asm/gic.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index a27da2c..02b1be1 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>  	return true;
>  }
>  
> +static bool test_targets(int nr_irqs)
> +{
> +	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
> +	u32 orig_targets;
> +	u32 cpu_mask;
> +	u32 pattern, reg;
> +
> +	orig_targets = readl(targetsptr + 32);
> +	report_prefix_push("ITARGETSR");
> +
> +	cpu_mask = (1 << nr_cpus) - 1;

Shouldn't this be 1 << (nr_cpus - 1) ?

Is this test always going to be gicv2-only? We should probably comment it,
if so. We don't want to risk this being run when nr_cpus can be larger
than 8.

> +	cpu_mask |= cpu_mask << 8;
> +	cpu_mask |= cpu_mask << 16;
> +
> +	/* Check that bits for non implemented CPUs are RAZ/WI. */
> +	if (nr_cpus < 8) {
> +		writel(0xffffffff, targetsptr + 32);
> +		report("bits for %d non-existent CPUs masked",
> +		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
> +	} else {
> +		report_skip("CPU masking (all CPUs implemented)");
> +	}
> +
> +	report("accesses beyond limit RAZ/WI",
> +	       test_readonly_32(targetsptr + nr_irqs, true));
> +
> +	pattern = 0x0103020f;
> +	writel(pattern, targetsptr + 32);
> +	reg = readl(targetsptr + 32);
> +	report("register content preserved (%08x => %08x)",
> +	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
> +
> +	/*
> +	 * The TARGETS registers are byte accessible, do a byte-wide
> +	 * read and write of known content to check for this.
> +	 */
> +	reg = readb(targetsptr + 33);
> +	report("byte reads successful (0x%08x => 0x%02x)",
> +	       reg == (BYTE(pattern, 1) & cpu_mask),
> +	       pattern & cpu_mask, reg);
> +
> +	pattern = REPLACE_BYTE(pattern, 2, 0x04);
> +	writeb(BYTE(pattern, 2), targetsptr + 34);
> +	reg = readl(targetsptr + 32);
> +	report("byte writes successful (0x%02x => 0x%08x)",
> +	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);

Last patch also had a byte addressability test. Maybe we should make
a helper function?

> +
> +	writel(orig_targets, targetsptr + 32);
> +	return true;

Function can/should be void.

> +}
> +
>  static int gic_test_mmio(int gic_version)
>  {
>  	u32 reg;
> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>  
>  	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>  
> +	if (gic_version == 2)
> +		test_targets(nr_irqs);
> +
>  	return 0;
>  }
>  
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index cef748d..6f170cb 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -14,6 +14,7 @@
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
>  #define GICD_IPRIORITYR			0x0400
> +#define GICD_ITARGETSR			0x0800
>  #define GICD_SGIR			0x0f00
>  #define GICD_ICPIDR2			0x0fe8
>  
> -- 
> 2.9.0
> 
>

Thanks,
drew

Re: [Qemu-devel] [kvm-unit-tests PATCH 4/4] arm/arm64: GICv3: add TYPER test

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:52PM +0000, Andre Przywara wrote:
> Add a simple test for the GICv3 TYPER test, which does only one basic
> check to ensure we have actually enough interrupt IDs if we support
> LPIs.
> Allow a GICv3 guest to do the common MMIO checks as well, where the
> register semantics are shared with a GICv2.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c         | 34 +++++++++++++++++++++++++++++++---
>  arm/unittests.cfg |  6 ++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 02b1be1..7de0e47 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -327,6 +327,30 @@ static bool test_typer_v2(uint32_t reg)
>  	return true;
>  }
>  
> +static bool test_typer_v3(uint32_t reg)
> +{
> +	int nr_intids;
> +
> +	report("GIC emulation %ssupport%s MBIs", 1,
> +	       reg & BIT(16) ? "" : "does not ",
> +	       reg & BIT(16) ? "s" : "");

Could just do the test once

 ("...%s...",  reg & BIT(16) ? "supports" : "does not support"

> +	report("GIC emulation %ssupport%s LPIs", 1,
> +	       reg & BIT(17) ? "" : "does not ",
> +	       reg & BIT(17) ? "s" : "");
> +	report("GIC emulation %ssupport%s Aff3", 1,
> +	       reg & BIT(24) ? "" : "does not ",
> +	       reg & BIT(24) ? "s" : "");
> +
> +	nr_intids = BIT(((reg >> 19) & 0x1f) + 1);
> +	report("%d interrupt IDs implemented", 1, nr_intids);
> +
> +	if (reg & BIT(17))
> +		report("%d LPIs supported", nr_intids > 8192,
> +		       nr_intids > 8192 ? nr_intids - 8192 : 0);

I'm wondering if we should try to keep the number of report lines
the same host to host. So anywhere we can't do a PASS/FAIL test we
should do a SKIP. Doing that will allow us to cleanly diff test
results between hosts. (I'm not sure I've been doing a good job of
that with the existing tests though...)

> +
> +	return true;

No need to return a value.

> +}
> +
>  #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
>  #define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
>  					((new) << ((byte) * 8)))
> @@ -460,8 +484,9 @@ static int gic_test_mmio(int gic_version)
>  		idreg = gic_dist_base + 0xfe8;
>  		break;
>  	case 0x3:
> -		report_abort("GICv3 MMIO tests NYI");
> -		return -1;
> +		gic_dist_base = gicv3_dist_base();
> +		idreg = gic_dist_base + 0xffe8;

No define for this ID reg?

> +		break;
>  	default:
>  		report_abort("GIC version %d not supported", gic_version);
>  		return 0;
> @@ -471,7 +496,10 @@ static int gic_test_mmio(int gic_version)
>  	nr_irqs = 32 * ((reg & 0x1f) + 1);
>  	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
>  
> -	test_typer_v2(reg);
> +	if (gic_version == 2)
> +		test_typer_v2(reg);
> +	else
> +		test_typer_v3(reg);

Maybe we should use a switch here too, preparing for v4

>  
>  	report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
>  
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 0162e5a..b432346 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -78,3 +78,9 @@ file = gic.flat
>  smp = $MAX_SMP
>  extra_params = -machine gic-version=3 -append 'ipi'
>  groups = gic
> +
> +[gicv3-mmio]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> -- 
> 2.9.0
> 
>

Thanks,
drew 

Re: [Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Auger Eric]

Hi Andre,

On 18/11/2016 14:06, Andrew Jones wrote:
> Hi Andre,
> 
> I'm so pleased to see this series. Thank you!
> 
> On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote:
>> This adds an MMIO subtest to the GIC test.
>> It accesses some generic GICv2 registers and does some sanity tests,
>> like checking for some of them being read-only.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c         | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg |  6 ++++
>>  lib/arm/asm/gic.h |  2 ++
>>  3 files changed, 107 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 638b8b1..ba2585b 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -3,6 +3,7 @@
>>   *
>>   * GICv2
>>   *   + test sending/receiving IPIs
>> + *   + MMIO access tests
>>   * GICv3
>>   *   + test sending/receiving IPIs
>>   *
>> @@ -274,6 +275,98 @@ static struct gic gicv3 = {
>>  	},
>>  };
>>  
>> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
>> +{
>> +	u32 reg;
>> +
>> +	writel(pattern, address);
>> +	reg = readl(address);
>> +
>> +	if (reg != orig)
>> +		writel(orig, address);
>> +
>> +	return reg == orig;
>> +}
>> +
>> +static bool test_readonly_32(void *address, bool razwi)
>> +{
>> +	u32 orig, pattern;
>> +
>> +	orig = readl(address);
>> +	if (razwi && orig)
>> +		return false;
>> +
>> +	pattern = 0xffffffff;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	pattern = 0xa5a55a5a;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	pattern = 0;
>> +	if (orig != pattern) {
>> +		if (!test_ro_pattern_32(address, pattern, orig))
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static bool test_typer_v2(uint32_t reg)
not: function name is a bit misleading, test_cpu_interfaces?

Thanks

Eric
>> +{
>> +	int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
>> +
>> +	report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
>> +	       nr_gic_cpus);
>> +
>> +	return true;
> 
> This test function can be a void.
> 
>> +}
>> +
>> +static int gic_test_mmio(int gic_version)
>> +{
>> +	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 + 0xfe8;
> 
> I see below you introduce GICD_ICPIDR2, so I guess you can use it here.
> 
>> +		break;
>> +	case 0x3:
>> +		report_abort("GICv3 MMIO tests NYI");
>> +		return -1;
> 
> can't reach this return
> 
>> +	default:
>> +		report_abort("GIC version %d not supported", gic_version);
>> +		return 0;
> 
> can't reach this return
> 
>> +	}
>> +
>> +	reg = readl(gic_dist_base + GICD_TYPER);
>> +	nr_irqs = 32 * ((reg & 0x1f) + 1);
> 
> Any reason to avoid using GICD_TYPER_IRQS() here?
> 
>> +	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
> 
> We usually just use printf for informational output (but we should
> probably add a 'report_info' in order to keep the prefixes. I can
> do that now.) Anyway, please s/1/true
> 
>> +
>> +	test_typer_v2(reg);
>> +
>> +	report("IIDR: 0x%x", 1, 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 (0x%x)",
>> +	       test_readonly_32(idreg, false),
>> +	       reg);
>> +
>> +	return 0;
> 
> You may want %08x for all your register printing.
> 
> Since you either abort or always return success, then this function can be
> a void.
> 
>> +}
>> +
>>  int main(int argc, char **argv)
>>  {
>>  	char pfx[8];
>> @@ -332,6 +425,12 @@ int main(int argc, char **argv)
>>  		}
>>  		ipi_test();
>>  
>> +	} else if (!strcmp(argv[1], "mmio")) {
>> +		report_prefix_push(argv[1]);
>> +
>> +		gic_test_mmio(gic_version());
> 
> Any reason to pass gic_version() here instead of just using it
> in gic_test_mmio?
> 
>> +
>> +		report_prefix_pop();
>>  	} else {
>>  		report_abort("Unknown subtest '%s'", argv[1]);
>>  	}
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index c7392c7..0162e5a 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>>  extra_params = -machine gic-version=2 -append 'ipi'
>>  groups = gic
>>  
>> +[gicv2-mmio]
>> +file = gic.flat
>> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>> +extra_params = -machine gic-version=2 -append 'mmio'
>> +groups = gic
>> +
>>  [gicv3-ipi]
>>  file = gic.flat
>>  smp = $MAX_SMP
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index c2267b6..cef748d 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -10,10 +10,12 @@
>>  /* Distributor registers */
>>  #define GICD_CTLR			0x0000
>>  #define GICD_TYPER			0x0004
>> +#define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>>  #define GICD_IPRIORITYR			0x0400
>>  #define GICD_SGIR			0x0f00
>> +#define GICD_ICPIDR2			0x0fe8
>>  
>>  #define GICD_TYPER_IRQS(typer)		((((typer) & 0x1f) + 1) * 32)
>>  #define GICD_INT_EN_SET_SGI		0x0000ffff
>> -- 
>> 2.9.0
>>
>>
> 
> Thanks,
> drew 
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 2/4] arm/arm64: GICv2: add GICD_IPRIORITYR testing

[Auger Eric]

Hi Andre,
On 18/11/2016 15:02, Andrew Jones wrote:
> On Thu, Nov 17, 2016 at 05:57:50PM +0000, Andre Przywara wrote:
>> Some tests for the IPRIORITY registers. The significant number of bits
>> is IMPLEMENTATION DEFINED, but should be the same for every IRQ.
>> Also these registers must be byte-accessible.
>> Check that accesses beyond the implemented IRQ limit are actually
>> read-as-zero/write-ignore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index ba2585b..a27da2c 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -327,6 +327,76 @@ static bool test_typer_v2(uint32_t reg)
>>  	return true;
>>  }
>>  
>> +#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
>> +#define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
>> +					((new) << ((byte) * 8)))
>> +
>> +static bool test_priorities(int nr_irqs, void *priptr)
>> +{
>> +	u32 orig_prio, reg, pri_bits;
>> +	u32 pri_mask, pattern;
>> +
>> +	orig_prio = readl(priptr + 32);
>> +	report_prefix_push("IPRIORITYR");
>> +
>> +	/*
>> +	 * Determine implemented number of priority bits by writing all 1's
>> +	 * and checking the number of cleared bits in the value read back.
>> +	 */
>> +	writel(0xffffffff, priptr + 32);
>> +	pri_mask = readl(priptr + 32);
>> +
>> +	reg = ~pri_mask;
>> +	report("consistent priority masking (0x%08x)",
>> +	       (((reg >> 16) == (reg & 0xffff)) &&
>> +	        ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
>> +
>> +	reg = reg & 0xff;
>> +	for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
>> +		;
>> +	report("implements at least 4 priority bits (%d)",
>> +	       pri_bits >= 4, pri_bits);
>> +
>> +	pattern = 0;
>> +	writel(pattern, priptr + 32);
>> +	report("clearing priorities", readl(priptr + 32) == pattern);
>> +
>> +	pattern = 0xffffffff;
>> +	writel(pattern, priptr + 32);
>> +	report("filling priorities",
>> +	       readl(priptr + 32) == (pattern & pri_mask));
what's the point to re-do that check?
>> +
>> +	report("accesses beyond limit RAZ/WI",
>> +	       test_readonly_32(priptr + nr_irqs, true));
>> +
>> +	writel(pattern, priptr + nr_irqs - 4);
>> +	report("accessing last SPIs",
>> +	       readl(priptr + nr_irqs - 4) == (pattern & pri_mask));
>> +
>> +	pattern = 0xff7fbf3f;
>> +	writel(pattern, priptr + 32);
>> +	report("priorities are preserved",
>> +	       readl(priptr + 32) == (pattern & pri_mask));
>> +
>> +	/*
>> +	 * The PRIORITY registers are byte accessible, do a byte-wide
>> +	 * read and write of known content to check for this.
>> +	 */
>> +	reg = readb(priptr + 33);
>> +	report("byte reads successful (0x%08x => 0x%02x)",
>> +	       reg == (BYTE(pattern, 1) & pri_mask), pattern & pri_mask, reg);
>> +
>> +	pattern = REPLACE_BYTE(pattern, 2, 0x1f);
>> +	writeb(BYTE(pattern, 2), priptr + 34);
>> +	reg = readl(priptr + 32);
>> +	report("byte writes successful (0x%02x => 0x%08x)",
>> +	       reg == (pattern & pri_mask), BYTE(pattern, 2) & pri_mask, reg);
>> +
>> +	report_prefix_pop();
>> +	writel(orig_prio, priptr + 32);
>> +	return true;
> 
> Might be nice to have FIRST_SPI and maybe LAST_SPI macros to avoid all
> the +32's
> 
> This function always returns true, so it can be a void.
> 
>> +}
>> +
>>  static int gic_test_mmio(int gic_version)
>>  {
>>  	u32 reg;
>> @@ -364,6 +434,8 @@ static int gic_test_mmio(int gic_version)
>>  	       test_readonly_32(idreg, false),
>>  	       reg);
>>  
>> +	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
> 
> Feel free to add state like nr_irqs and dist_base to the gic struct
> defined in arm/gic.c. That struct should provide the abstraction
> needed to handle both gicv2 and gicv3 and contain anything that the
> test functions need to refer to frequently. Using it should help
> reduce the amount of parameters passed around.
> 
>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 2.9.0
> 
> Otherwise looks good to me
same: Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>
>>
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Auger Eric]

Hi,

On 18/11/2016 15:20, Andrew Jones wrote:
> On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
>> Some tests for the ITARGETS registers.
>> Bits corresponding to non-existent CPUs must be RAZ/WI.
>> These registers must be byte-accessible, also check that accesses beyond
>> the implemented IRQ limit are actually read-as-zero/write-ignore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/arm/asm/gic.h |  1 +
>>  2 files changed, 55 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index a27da2c..02b1be1 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>>  	return true;
>>  }
>>  
>> +static bool test_targets(int nr_irqs)
>> +{
>> +	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
>> +	u32 orig_targets;
>> +	u32 cpu_mask;
>> +	u32 pattern, reg;
>> +
>> +	orig_targets = readl(targetsptr + 32);
>> +	report_prefix_push("ITARGETSR");
>> +
>> +	cpu_mask = (1 << nr_cpus) - 1;
> 
> Shouldn't this be 1 << (nr_cpus - 1) ?
original looks correct to me.
> 
> Is this test always going to be gicv2-only? We should probably comment it,
> if so. We don't want to risk this being run when nr_cpus can be larger
> than 8.
> 
>> +	cpu_mask |= cpu_mask << 8;
>> +	cpu_mask |= cpu_mask << 16;
Don't we miss the 4th byte mask?

Thanks

Eric
>> +
>> +	/* Check that bits for non implemented CPUs are RAZ/WI. */
>> +	if (nr_cpus < 8) {
>> +		writel(0xffffffff, targetsptr + 32);
>> +		report("bits for %d non-existent CPUs masked",
>> +		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
>> +	} else {
>> +		report_skip("CPU masking (all CPUs implemented)");
>> +	}
>> +
>> +	report("accesses beyond limit RAZ/WI",
>> +	       test_readonly_32(targetsptr + nr_irqs, true));
>> +
>> +	pattern = 0x0103020f;
>> +	writel(pattern, targetsptr + 32);
>> +	reg = readl(targetsptr + 32);
>> +	report("register content preserved (%08x => %08x)",
>> +	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
>> +
>> +	/*
>> +	 * The TARGETS registers are byte accessible, do a byte-wide
>> +	 * read and write of known content to check for this.
>> +	 */
>> +	reg = readb(targetsptr + 33);
>> +	report("byte reads successful (0x%08x => 0x%02x)",
>> +	       reg == (BYTE(pattern, 1) & cpu_mask),
>> +	       pattern & cpu_mask, reg);
>> +
>> +	pattern = REPLACE_BYTE(pattern, 2, 0x04);
>> +	writeb(BYTE(pattern, 2), targetsptr + 34);
>> +	reg = readl(targetsptr + 32);
>> +	report("byte writes successful (0x%02x => 0x%08x)",
>> +	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);
> 
> Last patch also had a byte addressability test. Maybe we should make
> a helper function?
> 
>> +
>> +	writel(orig_targets, targetsptr + 32);
>> +	return true;
> 
> Function can/should be void.
> 
>> +}
>> +
>>  static int gic_test_mmio(int gic_version)
>>  {
>>  	u32 reg;
>> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>>  
>>  	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>>  
>> +	if (gic_version == 2)
>> +		test_targets(nr_irqs);
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index cef748d..6f170cb 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -14,6 +14,7 @@
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>>  #define GICD_IPRIORITYR			0x0400
>> +#define GICD_ITARGETSR			0x0800
>>  #define GICD_SGIR			0x0f00
>>  #define GICD_ICPIDR2			0x0fe8
>>  
>> -- 
>> 2.9.0
>>
>>
> 
> Thanks,
> drew
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Auger Eric]

Hi Andre,

On 23/11/2016 14:24, Auger Eric wrote:
> Hi,
> 
> On 18/11/2016 15:20, Andrew Jones wrote:
>> On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
>>> Some tests for the ITARGETS registers.
>>> Bits corresponding to non-existent CPUs must be RAZ/WI.
>>> These registers must be byte-accessible, also check that accesses beyond
>>> the implemented IRQ limit are actually read-as-zero/write-ignore.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  lib/arm/asm/gic.h |  1 +
>>>  2 files changed, 55 insertions(+)
>>>
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index a27da2c..02b1be1 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>>>  	return true;
>>>  }
>>>  
>>> +static bool test_targets(int nr_irqs)
>>> +{
>>> +	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
>>> +	u32 orig_targets;
>>> +	u32 cpu_mask;
>>> +	u32 pattern, reg;
>>> +
>>> +	orig_targets = readl(targetsptr + 32);
>>> +	report_prefix_push("ITARGETSR");
>>> +
>>> +	cpu_mask = (1 << nr_cpus) - 1;
>>
>> Shouldn't this be 1 << (nr_cpus - 1) ?
> original looks correct to me.
>>
>> Is this test always going to be gicv2-only? We should probably comment it,
>> if so. We don't want to risk this being run when nr_cpus can be larger
>> than 8.
>>
>>> +	cpu_mask |= cpu_mask << 8;
>>> +	cpu_mask |= cpu_mask << 16;
> Don't we miss the 4th byte mask?
> 
> Thanks
> 
> Eric
>>> +
>>> +	/* Check that bits for non implemented CPUs are RAZ/WI. */
>>> +	if (nr_cpus < 8) {
>>> +		writel(0xffffffff, targetsptr + 32);
>>> +		report("bits for %d non-existent CPUs masked",
>>> +		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);

yep on AMD overdrive with smp=4 I get:

FAIL: gicv2: mmio: ITARGETSR: bits for 4 non-existent CPUs masked

Thanks

Eric

>>> +	} else {
>>> +		report_skip("CPU masking (all CPUs implemented)");
>>> +	}
>>> +
>>> +	report("accesses beyond limit RAZ/WI",
>>> +	       test_readonly_32(targetsptr + nr_irqs, true));
>>> +
>>> +	pattern = 0x0103020f;
>>> +	writel(pattern, targetsptr + 32);
>>> +	reg = readl(targetsptr + 32);
>>> +	report("register content preserved (%08x => %08x)",
>>> +	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
>>> +
>>> +	/*
>>> +	 * The TARGETS registers are byte accessible, do a byte-wide
>>> +	 * read and write of known content to check for this.
>>> +	 */
>>> +	reg = readb(targetsptr + 33);
>>> +	report("byte reads successful (0x%08x => 0x%02x)",
>>> +	       reg == (BYTE(pattern, 1) & cpu_mask),
>>> +	       pattern & cpu_mask, reg);
>>> +
>>> +	pattern = REPLACE_BYTE(pattern, 2, 0x04);
>>> +	writeb(BYTE(pattern, 2), targetsptr + 34);
>>> +	reg = readl(targetsptr + 32);
>>> +	report("byte writes successful (0x%02x => 0x%08x)",
>>> +	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);
>>
>> Last patch also had a byte addressability test. Maybe we should make
>> a helper function?
>>
>>> +
>>> +	writel(orig_targets, targetsptr + 32);
>>> +	return true;
>>
>> Function can/should be void.
>>
>>> +}
>>> +
>>>  static int gic_test_mmio(int gic_version)
>>>  {
>>>  	u32 reg;
>>> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>>>  
>>>  	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>>>  
>>> +	if (gic_version == 2)
>>> +		test_targets(nr_irqs);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>>> index cef748d..6f170cb 100644
>>> --- a/lib/arm/asm/gic.h
>>> +++ b/lib/arm/asm/gic.h
>>> @@ -14,6 +14,7 @@
>>>  #define GICD_IGROUPR			0x0080
>>>  #define GICD_ISENABLER			0x0100
>>>  #define GICD_IPRIORITYR			0x0400
>>> +#define GICD_ITARGETSR			0x0800
>>>  #define GICD_SGIR			0x0f00
>>>  #define GICD_ICPIDR2			0x0fe8
>>>  
>>> -- 
>>> 2.9.0
>>>
>>>
>>
>> Thanks,
>> drew
>>

Re: [Qemu-devel] [kvm-unit-tests PATCH 4/4] arm/arm64: GICv3: add TYPER test

[Auger Eric]

Hi,

On 18/11/2016 15:41, Andrew Jones wrote:
> On Thu, Nov 17, 2016 at 05:57:52PM +0000, Andre Przywara wrote:
>> Add a simple test for the GICv3 TYPER test, which does only one basic
>> check to ensure we have actually enough interrupt IDs if we support
>> LPIs.
>> Allow a GICv3 guest to do the common MMIO checks as well, where the
>> register semantics are shared with a GICv2.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/gic.c         | 34 +++++++++++++++++++++++++++++++---
>>  arm/unittests.cfg |  6 ++++++
>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
nit: in the top comments you could add "GICv3 MMIO test" for homogeneity
>> index 02b1be1..7de0e47 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -327,6 +327,30 @@ static bool test_typer_v2(uint32_t reg)
>>  	return true;
>>  }
>>  
>> +static bool test_typer_v3(uint32_t reg)
>> +{
>> +	int nr_intids;
>> +
>> +	report("GIC emulation %ssupport%s MBIs", 1,
>> +	       reg & BIT(16) ? "" : "does not ",
>> +	       reg & BIT(16) ? "s" : "");
> 
> Could just do the test once
> 
>  ("...%s...",  reg & BIT(16) ? "supports" : "does not support"
> 
>> +	report("GIC emulation %ssupport%s LPIs", 1,
>> +	       reg & BIT(17) ? "" : "does not ",
>> +	       reg & BIT(17) ? "s" : "");
>> +	report("GIC emulation %ssupport%s Aff3", 1,
>> +	       reg & BIT(24) ? "" : "does not ",
>> +	       reg & BIT(24) ? "s" : "");
>> +
>> +	nr_intids = BIT(((reg >> 19) & 0x1f) + 1);
>> +	report("%d interrupt IDs implemented", 1, nr_intids);
>> +
>> +	if (reg & BIT(17))
>> +		report("%d LPIs supported", nr_intids > 8192,

Maybe I misunderstand the spec but LPIs start at 8192 (>=) and also the
spec says that "In an implementation that includes LPIs, at least 8192
LPIs are supported (>= 2x 8192)"

Thanks

Eric

>> +		       nr_intids > 8192 ? nr_intids - 8192 : 0);
> 
> I'm wondering if we should try to keep the number of report lines
> the same host to host. So anywhere we can't do a PASS/FAIL test we
> should do a SKIP. Doing that will allow us to cleanly diff test
> results between hosts. (I'm not sure I've been doing a good job of
> that with the existing tests though...)
> 
>> +
>> +	return true;
> 
> No need to return a value.
> 
>> +}
>> +
>>  #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
>>  #define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\
>>  					((new) << ((byte) * 8)))
>> @@ -460,8 +484,9 @@ static int gic_test_mmio(int gic_version)
>>  		idreg = gic_dist_base + 0xfe8;
>>  		break;
>>  	case 0x3:
>> -		report_abort("GICv3 MMIO tests NYI");
>> -		return -1;
>> +		gic_dist_base = gicv3_dist_base();
>> +		idreg = gic_dist_base + 0xffe8;
> 
> No define for this ID reg?
> 
>> +		break;
>>  	default:
>>  		report_abort("GIC version %d not supported", gic_version);
>>  		return 0;
>> @@ -471,7 +496,10 @@ static int gic_test_mmio(int gic_version)
>>  	nr_irqs = 32 * ((reg & 0x1f) + 1);
>>  	report("number of implemented SPIs: %d", 1, nr_irqs - 32);
>>  
>> -	test_typer_v2(reg);
>> +	if (gic_version == 2)
>> +		test_typer_v2(reg);
>> +	else
>> +		test_typer_v3(reg);
> 
> Maybe we should use a switch here too, preparing for v4
> 
>>  
>>  	report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
>>  
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 0162e5a..b432346 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -78,3 +78,9 @@ file = gic.flat
>>  smp = $MAX_SMP
>>  extra_params = -machine gic-version=3 -append 'ipi'
>>  groups = gic
>> +
>> +[gicv3-mmio]
>> +file = gic.flat
>> +smp = $MAX_SMP
>> +extra_params = -machine gic-version=3 -append 'mmio'
>> +groups = gic
>> -- 
>> 2.9.0
>>
>>
> 
> Thanks,
> drew 
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Andre Przywara]

Hi Eric,

thanks for having such a close look (as always!).

On 23/11/16 13:51, Auger Eric wrote:
> Hi Andre,
> 
> On 23/11/2016 14:24, Auger Eric wrote:
>> Hi,
>>
>> On 18/11/2016 15:20, Andrew Jones wrote:
>>> On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
>>>> Some tests for the ITARGETS registers.
>>>> Bits corresponding to non-existent CPUs must be RAZ/WI.
>>>> These registers must be byte-accessible, also check that accesses beyond
>>>> the implemented IRQ limit are actually read-as-zero/write-ignore.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  lib/arm/asm/gic.h |  1 +
>>>>  2 files changed, 55 insertions(+)
>>>>
>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>> index a27da2c..02b1be1 100644
>>>> --- a/arm/gic.c
>>>> +++ b/arm/gic.c
>>>> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>>>>  	return true;
>>>>  }
>>>>  
>>>> +static bool test_targets(int nr_irqs)
>>>> +{
>>>> +	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
>>>> +	u32 orig_targets;
>>>> +	u32 cpu_mask;
>>>> +	u32 pattern, reg;
>>>> +
>>>> +	orig_targets = readl(targetsptr + 32);
>>>> +	report_prefix_push("ITARGETSR");
>>>> +
>>>> +	cpu_mask = (1 << nr_cpus) - 1;
>>>
>>> Shouldn't this be 1 << (nr_cpus - 1) ?
>> original looks correct to me.
>>>
>>> Is this test always going to be gicv2-only? We should probably comment it,
>>> if so. We don't want to risk this being run when nr_cpus can be larger
>>> than 8.
>>>
>>>> +	cpu_mask |= cpu_mask << 8;

So this instruction is supposed to copy bits[7:0] into bits[15:8] (not
caring about the other bits, which are zero anyway).

>>>> +	cpu_mask |= cpu_mask << 16;

And this one copies bits[15:0] into bits[31:16].

>> Don't we miss the 4th byte mask?

I don't think so, the idea is just to copy the lowest byte into all the
other three bytes of that word and thus to propagate the byte mask for
one IRQ into a word covering four interrupts. Does that make sense?
I take it this deserves a comment then ...

>>>> +
>>>> +	/* Check that bits for non implemented CPUs are RAZ/WI. */
>>>> +	if (nr_cpus < 8) {
>>>> +		writel(0xffffffff, targetsptr + 32);
>>>> +		report("bits for %d non-existent CPUs masked",
>>>> +		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
> 
> yep on AMD overdrive with smp=4 I get:
> 
> FAIL: gicv2: mmio: ITARGETSR: bits for 4 non-existent CPUs masked

I guess because you don't have
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/468296.html
in your host kernel?
This was one of the two genuine bugs I spotted with this.

Cheers,
Andre.

>>>> +	} else {
>>>> +		report_skip("CPU masking (all CPUs implemented)");
>>>> +	}
>>>> +
>>>> +	report("accesses beyond limit RAZ/WI",
>>>> +	       test_readonly_32(targetsptr + nr_irqs, true));
>>>> +
>>>> +	pattern = 0x0103020f;
>>>> +	writel(pattern, targetsptr + 32);
>>>> +	reg = readl(targetsptr + 32);
>>>> +	report("register content preserved (%08x => %08x)",
>>>> +	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
>>>> +
>>>> +	/*
>>>> +	 * The TARGETS registers are byte accessible, do a byte-wide
>>>> +	 * read and write of known content to check for this.
>>>> +	 */
>>>> +	reg = readb(targetsptr + 33);
>>>> +	report("byte reads successful (0x%08x => 0x%02x)",
>>>> +	       reg == (BYTE(pattern, 1) & cpu_mask),
>>>> +	       pattern & cpu_mask, reg);
>>>> +
>>>> +	pattern = REPLACE_BYTE(pattern, 2, 0x04);
>>>> +	writeb(BYTE(pattern, 2), targetsptr + 34);
>>>> +	reg = readl(targetsptr + 32);
>>>> +	report("byte writes successful (0x%02x => 0x%08x)",
>>>> +	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);
>>>
>>> Last patch also had a byte addressability test. Maybe we should make
>>> a helper function?
>>>
>>>> +
>>>> +	writel(orig_targets, targetsptr + 32);
>>>> +	return true;
>>>
>>> Function can/should be void.
>>>
>>>> +}
>>>> +
>>>>  static int gic_test_mmio(int gic_version)
>>>>  {
>>>>  	u32 reg;
>>>> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>>>>  
>>>>  	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>>>>  
>>>> +	if (gic_version == 2)
>>>> +		test_targets(nr_irqs);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>>>> index cef748d..6f170cb 100644
>>>> --- a/lib/arm/asm/gic.h
>>>> +++ b/lib/arm/asm/gic.h
>>>> @@ -14,6 +14,7 @@
>>>>  #define GICD_IGROUPR			0x0080
>>>>  #define GICD_ISENABLER			0x0100
>>>>  #define GICD_IPRIORITYR			0x0400
>>>> +#define GICD_ITARGETSR			0x0800
>>>>  #define GICD_SGIR			0x0f00
>>>>  #define GICD_ICPIDR2			0x0fe8
>>>>  
>>>> -- 
>>>> 2.9.0
>>>>
>>>>
>>>
>>> Thanks,
>>> drew
>>>

Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Auger Eric]

Hi Andre,
On 23/11/2016 15:13, Andre Przywara wrote:
> Hi Eric,
> 
> thanks for having such a close look (as always!).
> 
> On 23/11/16 13:51, Auger Eric wrote:
>> Hi Andre,
>>
>> On 23/11/2016 14:24, Auger Eric wrote:
>>> Hi,
>>>
>>> On 18/11/2016 15:20, Andrew Jones wrote:
>>>> On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
>>>>> Some tests for the ITARGETS registers.
>>>>> Bits corresponding to non-existent CPUs must be RAZ/WI.
>>>>> These registers must be byte-accessible, also check that accesses beyond
>>>>> the implemented IRQ limit are actually read-as-zero/write-ignore.
>>>>>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>>> ---
>>>>>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  lib/arm/asm/gic.h |  1 +
>>>>>  2 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>>> index a27da2c..02b1be1 100644
>>>>> --- a/arm/gic.c
>>>>> +++ b/arm/gic.c
>>>>> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>>>>>  	return true;
>>>>>  }
>>>>>  
>>>>> +static bool test_targets(int nr_irqs)
>>>>> +{
>>>>> +	void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
>>>>> +	u32 orig_targets;
>>>>> +	u32 cpu_mask;
>>>>> +	u32 pattern, reg;
>>>>> +
>>>>> +	orig_targets = readl(targetsptr + 32);
>>>>> +	report_prefix_push("ITARGETSR");
>>>>> +
>>>>> +	cpu_mask = (1 << nr_cpus) - 1;
>>>>
>>>> Shouldn't this be 1 << (nr_cpus - 1) ?
>>> original looks correct to me.
>>>>
>>>> Is this test always going to be gicv2-only? We should probably comment it,
>>>> if so. We don't want to risk this being run when nr_cpus can be larger
>>>> than 8.
>>>>
>>>>> +	cpu_mask |= cpu_mask << 8;
> 
> So this instruction is supposed to copy bits[7:0] into bits[15:8] (not
> caring about the other bits, which are zero anyway).
> 
>>>>> +	cpu_mask |= cpu_mask << 16;
> 
> And this one copies bits[15:0] into bits[31:16].
> 
>>> Don't we miss the 4th byte mask?
> 
> I don't think so, the idea is just to copy the lowest byte into all the
> other three bytes of that word and thus to propagate the byte mask for
> one IRQ into a word covering four interrupts. Does that make sense?

Hum yes that's fully correct. Sorry for the noise.
> I take it this deserves a comment then ...
> 
>>>>> +
>>>>> +	/* Check that bits for non implemented CPUs are RAZ/WI. */
>>>>> +	if (nr_cpus < 8) {
>>>>> +		writel(0xffffffff, targetsptr + 32);
>>>>> +		report("bits for %d non-existent CPUs masked",
>>>>> +		       !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
>>
>> yep on AMD overdrive with smp=4 I get:
>>
>> FAIL: gicv2: mmio: ITARGETSR: bits for 4 non-existent CPUs masked
> 
> I guess because you don't have
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/468296.html
> in your host kernel?
> This was one of the two genuine bugs I spotted with this.

Ah ok cool!

Cheers

Eric
> 
> Cheers,
> Andre.
> 
>>>>> +	} else {
>>>>> +		report_skip("CPU masking (all CPUs implemented)");
>>>>> +	}
>>>>> +
>>>>> +	report("accesses beyond limit RAZ/WI",
>>>>> +	       test_readonly_32(targetsptr + nr_irqs, true));
>>>>> +
>>>>> +	pattern = 0x0103020f;
>>>>> +	writel(pattern, targetsptr + 32);
>>>>> +	reg = readl(targetsptr + 32);
>>>>> +	report("register content preserved (%08x => %08x)",
>>>>> +	       reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
>>>>> +
>>>>> +	/*
>>>>> +	 * The TARGETS registers are byte accessible, do a byte-wide
>>>>> +	 * read and write of known content to check for this.
>>>>> +	 */
>>>>> +	reg = readb(targetsptr + 33);
>>>>> +	report("byte reads successful (0x%08x => 0x%02x)",
>>>>> +	       reg == (BYTE(pattern, 1) & cpu_mask),
>>>>> +	       pattern & cpu_mask, reg);
>>>>> +
>>>>> +	pattern = REPLACE_BYTE(pattern, 2, 0x04);
>>>>> +	writeb(BYTE(pattern, 2), targetsptr + 34);
>>>>> +	reg = readl(targetsptr + 32);
>>>>> +	report("byte writes successful (0x%02x => 0x%08x)",
>>>>> +	       reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);
>>>>
>>>> Last patch also had a byte addressability test. Maybe we should make
>>>> a helper function?
>>>>
>>>>> +
>>>>> +	writel(orig_targets, targetsptr + 32);
>>>>> +	return true;
>>>>
>>>> Function can/should be void.
>>>>
>>>>> +}
>>>>> +
>>>>>  static int gic_test_mmio(int gic_version)
>>>>>  {
>>>>>  	u32 reg;
>>>>> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>>>>>  
>>>>>  	test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>>>>>  
>>>>> +	if (gic_version == 2)
>>>>> +		test_targets(nr_irqs);
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>>>>> index cef748d..6f170cb 100644
>>>>> --- a/lib/arm/asm/gic.h
>>>>> +++ b/lib/arm/asm/gic.h
>>>>> @@ -14,6 +14,7 @@
>>>>>  #define GICD_IGROUPR			0x0080
>>>>>  #define GICD_ISENABLER			0x0100
>>>>>  #define GICD_IPRIORITYR			0x0400
>>>>> +#define GICD_ITARGETSR			0x0800
>>>>>  #define GICD_SGIR			0x0f00
>>>>>  #define GICD_ICPIDR2			0x0fe8
>>>>>  
>>>>> -- 
>>>>> 2.9.0
>>>>>
>>>>>
>>>>
>>>> Thanks,
>>>> drew
>>>>
>