arm: Add PL031 test

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Signed-off-by Alexander Graf <[email protected]>

---
 arm/Makefile.common |   1 +
 arm/pl031.c         | 227 ++++++++++++++++++++++++++++++++++++++++++++
 lib/arm/asm/gic.h   |   1 +
 3 files changed, 229 insertions(+)
 create mode 100644 arm/pl031.c

arm/Makefile.common +1 -0

index f0c4b5d..b8988f2 100644
@@ -11,6 +11,7 @@ tests-common += $(TEST_DIR)/pmu.flat
 tests-common += $(TEST_DIR)/gic.flat
 tests-common += $(TEST_DIR)/psci.flat
 tests-common += $(TEST_DIR)/sieve.flat
+tests-common += $(TEST_DIR)/pl031.flat
 
 tests-all = $(tests-common) $(tests)
 all: directories $(tests-all)

arm/pl031.c new file +9 -0

new file mode 100644
index 0000000..a364a1a
@@ -0,0 +1,227 @@
+/*
+ * Verify PL031 functionality
+ *
+ * This test verifies whether the emulated PL031 behaves correctly.
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates.
+ * Author: Alexander Graf <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.

arm/pl031.c new file +82 -0

+
+struct pl031_regs {
+	uint32_t dr;	/* Data Register */
+	uint32_t mr;	/* Match Register */
+	uint32_t lr;	/* Load Register */
+	union {
+		uint8_t cr;	/* Control Register */
+		uint32_t cr32;
+	};
+	union {
+		uint8_t imsc;	/* Interrupt Mask Set or Clear register */
+		uint32_t imsc32;
+	};
+	union {
+		uint8_t ris;	/* Raw Interrupt Status */
+		uint32_t ris32;
+	};
+	union {
+		uint8_t mis;	/* Masked Interrupt Status */
+		uint32_t mis32;
+	};
+	union {
+		uint8_t icr;	/* Interrupt Clear Register */
+		uint32_t icr32;
+	};
+	uint32_t reserved[1008];
+	uint32_t periph_id[4];
+	uint32_t pcell_id[4];
+};
+
+static struct pl031_regs *pl031 = (void*)PL031_BASE;
+static void *gic_ispendr;
+static void *gic_isenabler;
+static bool irq_triggered;
+
+static int check_id(void)
+{
+	uint32_t id[] = { 0x31, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(id); i++)
+		if (id[i] != readl(&pl031->periph_id[i]))
+			return 1;
+
+	return 0;
+}
+
+static int check_ro(void)
+{
+	uint32_t offs[] = { offsetof(struct pl031_regs, ris),
+			    offsetof(struct pl031_regs, mis),
+			    offsetof(struct pl031_regs, periph_id[0]),
+			    offsetof(struct pl031_regs, periph_id[1]),
+			    offsetof(struct pl031_regs, periph_id[2]),
+			    offsetof(struct pl031_regs, periph_id[3]),
+			    offsetof(struct pl031_regs, pcell_id[0]),
+			    offsetof(struct pl031_regs, pcell_id[1]),
+			    offsetof(struct pl031_regs, pcell_id[2]),
+			    offsetof(struct pl031_regs, pcell_id[3]) };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(offs); i++) {
+		uint32_t before32;
+		uint16_t before16;
+		uint8_t before8;
+		void *addr = (void*)pl031 + offs[i];
+		uint32_t poison = 0xdeadbeefULL;
+
+		before8 = readb(addr);
+		before16 = readw(addr);
+		before32 = readl(addr);
+
+		writeb(poison, addr);
+		writew(poison, addr);
+		writel(poison, addr);
+
+		if (before8 != readb(addr))
+			return 1;
+		if (before16 != readw(addr))
+			return 1;
+		if (before32 != readl(addr))
+			return 1;

arm/pl031.c new file +12 -0

+
+	if (irqnr == SPI(PL031_IRQ)) {
+		report("  RTC RIS == 1", readl(&pl031->ris) == 1);
+		report("  RTC MIS == 1", readl(&pl031->mis) == 1);
+
+		/* Writing any value should clear IRQ status */
+		writel(0x80000000ULL, &pl031->icr);
+
+		report("  RTC RIS == 0", readl(&pl031->ris) == 0);
+		report("  RTC MIS == 0", readl(&pl031->mis) == 0);
+		irq_triggered = true;
+	} else {

arm/pl031.c new file +39 -0

+	uint64_t target_tick = before_tick + (cntfrq * (seconds_to_wait + 1));
+
+	report_info("Checking IRQ trigger (MR)");
+
+	irq_triggered = false;
+
+	/* Fire IRQ in 1 second */
+	writel(before + seconds_to_wait, &pl031->mr);
+
+	install_irq_handler(EL1H_IRQ, irq_handler);
+
+	/* Wait until 2 seconds are over */
+	while (read_sysreg(cntpct_el0) < target_tick) ;
+
+	report("  RTC IRQ not delivered without mask", !gic_irq_pending());
+
+	/* Mask the IRQ so that it gets delivered */
+	writel(1, &pl031->imsc);
+	report("  RTC IRQ pending now", gic_irq_pending());
+
+	/* Enable retrieval of IRQ */
+	gic_irq_unmask();
+	local_irq_enable();
+
+	report("  IRQ triggered", irq_triggered);
+	report("  RTC IRQ not pending anymore", !gic_irq_pending());
+	if (!irq_triggered) {
+		report_info("  RTC RIS: %x", readl(&pl031->ris));
+		report_info("  RTC MIS: %x", readl(&pl031->mis));
+		report_info("  RTC IMSC: %x", readl(&pl031->imsc));
+		report_info("  GIC IRQs pending: %08x %08x", readl(gic_ispendr), readl(gic_ispendr + 4));
+	}
+
+	local_irq_disable();
+	return 0;
+}
+
+static void rtc_irq_init(void)
+{

arm/pl031.c new file +17 -0

+		break;
+	}
+}
+
+int main(int argc, char **argv)
+{
+	cntfrq = get_cntfrq();
+	rtc_irq_init();
+
+	report("Periph/PCell IDs match", !check_id());
+	report("R/O fields are R/O", !check_ro());
+	report("RTC ticks at 1HZ", !check_rtc_freq());
+	report("RTC IRQ not pending yet", !gic_irq_pending());
+	check_rtc_irq();
+
+	return report_summary();
+}

lib/arm/asm/gic.h +0 -0

index f6dfb90..1fc10a0 100644

Hi Alex,

I don't know much about pl031, so my comments are pretty general...

Quoted 45 lines · Alexander Graf

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Signed-off-by Alexander Graf <[email protected]>

---
  arm/Makefile.common |   1 +
  arm/pl031.c         | 227 ++++++++++++++++++++++++++++++++++++++++++++
  lib/arm/asm/gic.h   |   1 +
  3 files changed, 229 insertions(+)

create mode 100644 arm/pl031.c

arm/Makefile.common +1 -0

index f0c4b5d..b8988f2 100644
@@ -11,6 +11,7 @@ tests-common += $(TEST_DIR)/pmu.flat
  tests-common += $(TEST_DIR)/gic.flat
  tests-common += $(TEST_DIR)/psci.flat
  tests-common += $(TEST_DIR)/sieve.flat
+tests-common += $(TEST_DIR)/pl031.flat
 
  tests-all = $(tests-common) $(tests)
  all: directories $(tests-all)

arm/pl031.c new file +14 -0

new file mode 100644
index 0000000..a364a1a
@@ -0,0 +1,227 @@
+/*
+ * Verify PL031 functionality
+ *
+ * This test verifies whether the emulated PL031 behaves correctly.
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates.
+ * Author: Alexander Graf <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <asm/processor.h>
+#include <asm/io.h>
+#include <asm/gic.h>

arm/pl031.c line 19

Quoted 5 lines

arm/pl031.c new file +5 -0

+
+static u32 cntfrq;
+
+#define PL031_BASE 0x09010000
+#define PL031_IRQ 2

Does the unit test framework have a way to extract this from the firmware tables? That'd be much nicer...

Quoted 83 lines

arm/pl031.c new file +82 -0

+
+struct pl031_regs {
+	uint32_t dr;	/* Data Register */
+	uint32_t mr;	/* Match Register */
+	uint32_t lr;	/* Load Register */
+	union {
+		uint8_t cr;	/* Control Register */
+		uint32_t cr32;
+	};
+	union {
+		uint8_t imsc;	/* Interrupt Mask Set or Clear register */
+		uint32_t imsc32;
+	};
+	union {
+		uint8_t ris;	/* Raw Interrupt Status */
+		uint32_t ris32;
+	};
+	union {
+		uint8_t mis;	/* Masked Interrupt Status */
+		uint32_t mis32;
+	};
+	union {
+		uint8_t icr;	/* Interrupt Clear Register */
+		uint32_t icr32;
+	};
+	uint32_t reserved[1008];
+	uint32_t periph_id[4];
+	uint32_t pcell_id[4];
+};
+
+static struct pl031_regs *pl031 = (void*)PL031_BASE;
+static void *gic_ispendr;
+static void *gic_isenabler;
+static bool irq_triggered;
+
+static int check_id(void)
+{
+	uint32_t id[] = { 0x31, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(id); i++)
+		if (id[i] != readl(&pl031->periph_id[i]))
+			return 1;
+
+	return 0;
+}
+
+static int check_ro(void)
+{
+	uint32_t offs[] = { offsetof(struct pl031_regs, ris),
+			    offsetof(struct pl031_regs, mis),
+			    offsetof(struct pl031_regs, periph_id[0]),
+			    offsetof(struct pl031_regs, periph_id[1]),
+			    offsetof(struct pl031_regs, periph_id[2]),
+			    offsetof(struct pl031_regs, periph_id[3]),
+			    offsetof(struct pl031_regs, pcell_id[0]),
+			    offsetof(struct pl031_regs, pcell_id[1]),
+			    offsetof(struct pl031_regs, pcell_id[2]),
+			    offsetof(struct pl031_regs, pcell_id[3]) };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(offs); i++) {
+		uint32_t before32;
+		uint16_t before16;
+		uint8_t before8;
+		void *addr = (void*)pl031 + offs[i];
+		uint32_t poison = 0xdeadbeefULL;
+
+		before8 = readb(addr);
+		before16 = readw(addr);
+		before32 = readl(addr);
+
+		writeb(poison, addr);
+		writew(poison, addr);
+		writel(poison, addr);
+
+		if (before8 != readb(addr))
+			return 1;
+		if (before16 != readw(addr))
+			return 1;
+		if (before32 != readl(addr))
+			return 1;

arm/pl031.c line 111

Quoted 10 lines

arm/pl031.c new file +10 -0

+	}
+
+	return 0;
+}
+
+static int check_rtc_freq(void)
+{
+	uint32_t seconds_to_wait = 2;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);

Hmmm. See below.

arm/pl031.c line 115

Quoted 4 lines

arm/pl031.c new file +4 -0

+	uint64_t target_tick = before_tick + (cntfrq * seconds_to_wait);
+
+	/* Wait for 2 seconds */
+	while (read_sysreg(cntpct_el0) < target_tick) ;

Careful here. The control dependency saves you, but in general we want to guarantee some stronger ordering when it comes to the counter. The Linux kernel has the following:

/*
* Ensure that reads of the counter are treated the same as memory reads
* for the purposes of ordering by subsequent memory barriers.
*
* This insanity brought to you by speculative system register reads,
* out-of-order memory accesses, sequence locks and Thomas Gleixner. *
* http://lists.infradead.org/pipermail/linux-arm-kernel/2019-February/631195.html
*/

#define arch_counter_enforce_ordering(val) do {                         \
         u64 tmp, _val = (val);                                          \
                                                                         \
         asm volatile(                                                   \
         "       eor     %0, %1, %1\n"                                   \
         "       add     %0, sp, %0\n"                                   \
         "       ldr     xzr, [%0]"                                      \
         : "=r" (tmp) : "r" (_val));                                     \
} while (0)

static __always_inline u64 __arch_counter_get_cntpct_stable(void)

{
         u64 cnt;

         isb();
         cnt = arch_timer_reg_read_stable(cntpct_el0);
         arch_counter_enforce_ordering(cnt);
         return cnt;
}

The initial ISB prevents the counter read from being reordered with earlier instructions, while the fake memory dependency enforces ordering with subsequent memory accesses. You're welcome ;-).

arm/pl031.c line 125

Quoted 11 lines

arm/pl031.c new file +10 -0

+
+	if (readl(&pl031->dr) != before + seconds_to_wait)
+		return 1;
+
+	return 0;
+}
+
+static bool gic_irq_pending(void)
+{
+	return readl(gic_ispendr + 4) & (1 << (SPI(PL031_IRQ) - 32));

nit: you may want to make the offset depend on the interrupt itself...

arm/pl031.c line 135

Quoted 11 lines

arm/pl031.c new file +10 -0

+}
+
+static void gic_irq_unmask(void)
+{
+	writel(1 << (SPI(PL031_IRQ) - 32), gic_isenabler + 4);
+}
+
+static void irq_handler(struct pt_regs *regs)
+{
+	u32 irqstat = gic_read_iar();

GICv3 requires a DSB SY after reading IAR. Is that included in gic_read_iar()?

arm/pl031.c line 139

Quoted 5 lines

arm/pl031.c new file +4 -0

+	u32 irqnr = gic_iar_irqnr(irqstat);
+
+	if (irqnr != GICC_INT_SPURIOUS)
+		gic_write_eoir(irqstat);

You can always EOI spurious. Are you guaranteed to be in EOImode==0? If not, you'd need an extra write to DIR. Also, this needs an ISB

for GICv3.

Quoted 13 lines

arm/pl031.c new file +12 -0

+
+	if (irqnr == SPI(PL031_IRQ)) {
+		report("  RTC RIS == 1", readl(&pl031->ris) == 1);
+		report("  RTC MIS == 1", readl(&pl031->mis) == 1);
+
+		/* Writing any value should clear IRQ status */
+		writel(0x80000000ULL, &pl031->icr);
+
+		report("  RTC RIS == 0", readl(&pl031->ris) == 0);
+		report("  RTC MIS == 0", readl(&pl031->mis) == 0);
+		irq_triggered = true;
+	} else {

arm/pl031.c line 161

Quoted 10 lines

arm/pl031.c new file +10 -0

+		report_info("Unexpected interrupt: %d\n", irqnr);
+		return;
+	}
+}
+
+static int check_rtc_irq(void)
+{
+	uint32_t seconds_to_wait = 1;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);

Same problem as above.

Quoted 44 lines

arm/pl031.c new file +44 -0

+	uint64_t target_tick = before_tick + (cntfrq * (seconds_to_wait + 1));
+
+	report_info("Checking IRQ trigger (MR)");
+
+	irq_triggered = false;
+
+	/* Fire IRQ in 1 second */
+	writel(before + seconds_to_wait, &pl031->mr);
+
+	install_irq_handler(EL1H_IRQ, irq_handler);
+
+	/* Wait until 2 seconds are over */
+	while (read_sysreg(cntpct_el0) < target_tick) ;
+
+	report("  RTC IRQ not delivered without mask", !gic_irq_pending());
+
+	/* Mask the IRQ so that it gets delivered */
+	writel(1, &pl031->imsc);
+	report("  RTC IRQ pending now", gic_irq_pending());
+
+	/* Enable retrieval of IRQ */
+	gic_irq_unmask();
+	local_irq_enable();
+
+	report("  IRQ triggered", irq_triggered);
+	report("  RTC IRQ not pending anymore", !gic_irq_pending());
+	if (!irq_triggered) {
+		report_info("  RTC RIS: %x", readl(&pl031->ris));
+		report_info("  RTC MIS: %x", readl(&pl031->mis));
+		report_info("  RTC IMSC: %x", readl(&pl031->imsc));
+		report_info("  GIC IRQs pending: %08x %08x", readl(gic_ispendr), readl(gic_ispendr + 4));
+	}
+
+	local_irq_disable();
+	return 0;
+}
+
+static void rtc_irq_init(void)
+{
+	gic_enable_defaults();
+
+	switch (gic_version()) {
+	case 2:
+		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;

arm/pl031.c line 210

Quoted 5 lines

arm/pl031.c new file +5 -0

+		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
+		break;
+	case 3:
+		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+		gic_isenabler = gicv3_sgi_base() + GICD_ISENABLER;

This only points to SGIs and PPIs. How does it work on GICv3? Let me guess: you haven't tested it... ;-)

Quoted 50 lines · Marc Zyngier

Hi Alex,

I don't know much about pl031, so my comments are pretty general...

Quoted 45 lines · Alexander Graf

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Signed-off-by Alexander Graf <[email protected]>

---
  arm/Makefile.common |   1 +
  arm/pl031.c         | 227 ++++++++++++++++++++++++++++++++++++++++++++
  lib/arm/asm/gic.h   |   1 +
  3 files changed, 229 insertions(+)

create mode 100644 arm/pl031.c

arm/Makefile.common +1 -0

index f0c4b5d..b8988f2 100644
@@ -11,6 +11,7 @@ tests-common += $(TEST_DIR)/pmu.flat
  tests-common += $(TEST_DIR)/gic.flat
  tests-common += $(TEST_DIR)/psci.flat
  tests-common += $(TEST_DIR)/sieve.flat
+tests-common += $(TEST_DIR)/pl031.flat
 
  tests-all = $(tests-common) $(tests)
  all: directories $(tests-all)

arm/pl031.c new file +14 -0

new file mode 100644
index 0000000..a364a1a
@@ -0,0 +1,227 @@
+/*
+ * Verify PL031 functionality
+ *
+ * This test verifies whether the emulated PL031 behaves correctly.
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates.
+ * Author: Alexander Graf <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <asm/processor.h>
+#include <asm/io.h>
+#include <asm/gic.h>

Quoted 8 lines

arm/pl031.c line 19

Quoted 5 lines

arm/pl031.c new file +5 -0

+
+static u32 cntfrq;
+
+#define PL031_BASE 0x09010000
+#define PL031_IRQ 2

Does the unit test framework have a way to extract this from the firmware tables? That'd be much nicer...

It does, I've moved it to that now :).

Quoted 134 lines

Quoted 83 lines

arm/pl031.c new file +82 -0

+
+struct pl031_regs {
+	uint32_t dr;	/* Data Register */
+	uint32_t mr;	/* Match Register */
+	uint32_t lr;	/* Load Register */
+	union {
+		uint8_t cr;	/* Control Register */
+		uint32_t cr32;
+	};
+	union {
+		uint8_t imsc;	/* Interrupt Mask Set or Clear register */
+		uint32_t imsc32;
+	};
+	union {
+		uint8_t ris;	/* Raw Interrupt Status */
+		uint32_t ris32;
+	};
+	union {
+		uint8_t mis;	/* Masked Interrupt Status */
+		uint32_t mis32;
+	};
+	union {
+		uint8_t icr;	/* Interrupt Clear Register */
+		uint32_t icr32;
+	};
+	uint32_t reserved[1008];
+	uint32_t periph_id[4];
+	uint32_t pcell_id[4];
+};
+
+static struct pl031_regs *pl031 = (void*)PL031_BASE;
+static void *gic_ispendr;
+static void *gic_isenabler;
+static bool irq_triggered;
+
+static int check_id(void)
+{
+	uint32_t id[] = { 0x31, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(id); i++)
+		if (id[i] != readl(&pl031->periph_id[i]))
+			return 1;
+
+	return 0;
+}
+
+static int check_ro(void)
+{
+	uint32_t offs[] = { offsetof(struct pl031_regs, ris),
+			    offsetof(struct pl031_regs, mis),
+			    offsetof(struct pl031_regs, periph_id[0]),
+			    offsetof(struct pl031_regs, periph_id[1]),
+			    offsetof(struct pl031_regs, periph_id[2]),
+			    offsetof(struct pl031_regs, periph_id[3]),
+			    offsetof(struct pl031_regs, pcell_id[0]),
+			    offsetof(struct pl031_regs, pcell_id[1]),
+			    offsetof(struct pl031_regs, pcell_id[2]),
+			    offsetof(struct pl031_regs, pcell_id[3]) };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(offs); i++) {
+		uint32_t before32;
+		uint16_t before16;
+		uint8_t before8;
+		void *addr = (void*)pl031 + offs[i];
+		uint32_t poison = 0xdeadbeefULL;
+
+		before8 = readb(addr);
+		before16 = readw(addr);
+		before32 = readl(addr);
+
+		writeb(poison, addr);
+		writew(poison, addr);
+		writel(poison, addr);
+
+		if (before8 != readb(addr))
+			return 1;
+		if (before16 != readw(addr))
+			return 1;
+		if (before32 != readl(addr))
+			return 1;

arm/pl031.c line 111

Quoted 10 lines

arm/pl031.c new file +10 -0

+	}
+
+	return 0;
+}
+
+static int check_rtc_freq(void)
+{
+	uint32_t seconds_to_wait = 2;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);

Hmmm. See below.

arm/pl031.c line 115

Quoted 4 lines

arm/pl031.c new file +4 -0

+	uint64_t target_tick = before_tick + (cntfrq * seconds_to_wait);
+
+	/* Wait for 2 seconds */
+	while (read_sysreg(cntpct_el0) < target_tick) ;

Careful here. The control dependency saves you, but in general we want to guarantee some stronger ordering when it comes to the counter. The Linux kernel has the following:

/*
* Ensure that reads of the counter are treated the same as memory reads
* for the purposes of ordering by subsequent memory barriers.
*
* This insanity brought to you by speculative system register reads,
* out-of-order memory accesses, sequence locks and Thomas Gleixner. *
* http://lists.infradead.org/pipermail/linux-arm-kernel/2019-February/631195.html
*/

#define arch_counter_enforce_ordering(val) do {                         \
         u64 tmp, _val = (val);                                          \
                                                                         \
         asm volatile(                                                   \
         "       eor     %0, %1, %1\n"                                   \
         "       add     %0, sp, %0\n"                                   \
         "       ldr     xzr, [%0]"                                      \
         : "=r" (tmp) : "r" (_val));                                     \
} while (0)

static __always_inline u64 __arch_counter_get_cntpct_stable(void)

{
         u64 cnt;

         isb();
         cnt = arch_timer_reg_read_stable(cntpct_el0);
         arch_counter_enforce_ordering(cnt);
         return cnt;
}

Quoted 3 lines

The initial ISB prevents the counter read from being reordered with earlier instructions, while the fake memory dependency enforces ordering with subsequent memory accesses. You're welcome ;-).

Do we really care enough in this case? It seems a bit overkill. The worst thing that happens is that we wiggle around the 2 second mark a bit and don't hit it right on the spot, no?

Quoted 14 lines

arm/pl031.c line 125

Quoted 12 lines

arm/pl031.c new file +10 -0

+
+	if (readl(&pl031->dr) != before + seconds_to_wait)
+		return 1;
+
+	return 0;
+}
+
+static bool gic_irq_pending(void)
+{
+	return readl(gic_ispendr + 4) & (1 << (SPI(PL031_IRQ) - 32));

nit: you may want to make the offset depend on the interrupt itself...

Yes, definitely :).

Quoted 13 lines

arm/pl031.c line 135

Quoted 11 lines

arm/pl031.c new file +10 -0

+}
+
+static void gic_irq_unmask(void)
+{
+	writel(1 << (SPI(PL031_IRQ) - 32), gic_isenabler + 4);
+}
+
+static void irq_handler(struct pt_regs *regs)
+{
+	u32 irqstat = gic_read_iar();

GICv3 requires a DSB SY after reading IAR. Is that included in gic_read_iar()?

Looks like it:

static inline u32 gicv3_read_iar(void)

{
         u64 irqstat;
         asm volatile("mrs_s %0, " xstr(ICC_IAR1_EL1) : "=r" (irqstat));
         dsb(sy);
         return (u64)irqstat;
}

Looks like Andre reviewed the code that introduced the GIC logic well :).

Quoted 9 lines

arm/pl031.c line 139

Quoted 6 lines

arm/pl031.c new file +4 -0

+	u32 irqnr = gic_iar_irqnr(irqstat);
+
+	if (irqnr != GICC_INT_SPURIOUS)
+		gic_write_eoir(irqstat);

You can always EOI spurious. Are you guaranteed to be in EOImode==0? If not, you'd need an extra write to DIR. Also, this needs an ISB

I copied that bit from the timer test. I'll be happy to remove the spurious check though.

We have the following in gicv3_enable_defaults() which IIUC clears the EOImode bit, right?

writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A |
GICD_CTLR_ENABLE_G1,

                dist + GICD_CTLR);

Quoted 1 line

for GICv3.

The ISB is there already:

static inline void gicv3_write_eoir(u32 irq)

{
         asm volatile("msr_s " xstr(ICC_EOIR1_EL1) ", %0" : : "r" 
((u64)irq));
         isb();
}

Quoted 71 lines

Quoted 14 lines

arm/pl031.c new file +12 -0

+
+	if (irqnr == SPI(PL031_IRQ)) {
+		report("  RTC RIS == 1", readl(&pl031->ris) == 1);
+		report("  RTC MIS == 1", readl(&pl031->mis) == 1);
+
+		/* Writing any value should clear IRQ status */
+		writel(0x80000000ULL, &pl031->icr);
+
+		report("  RTC RIS == 0", readl(&pl031->ris) == 0);
+		report("  RTC MIS == 0", readl(&pl031->mis) == 0);
+		irq_triggered = true;
+	} else {

arm/pl031.c line 161

Quoted 10 lines

arm/pl031.c new file +10 -0

+		report_info("Unexpected interrupt: %d\n", irqnr);
+		return;
+	}
+}
+
+static int check_rtc_irq(void)
+{
+	uint32_t seconds_to_wait = 1;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);

Same problem as above.

Quoted 44 lines

arm/pl031.c new file +44 -0

+	uint64_t target_tick = before_tick + (cntfrq * (seconds_to_wait + 1));
+
+	report_info("Checking IRQ trigger (MR)");
+
+	irq_triggered = false;
+
+	/* Fire IRQ in 1 second */
+	writel(before + seconds_to_wait, &pl031->mr);
+
+	install_irq_handler(EL1H_IRQ, irq_handler);
+
+	/* Wait until 2 seconds are over */
+	while (read_sysreg(cntpct_el0) < target_tick) ;
+
+	report("  RTC IRQ not delivered without mask", !gic_irq_pending());
+
+	/* Mask the IRQ so that it gets delivered */
+	writel(1, &pl031->imsc);
+	report("  RTC IRQ pending now", gic_irq_pending());
+
+	/* Enable retrieval of IRQ */
+	gic_irq_unmask();
+	local_irq_enable();
+
+	report("  IRQ triggered", irq_triggered);
+	report("  RTC IRQ not pending anymore", !gic_irq_pending());
+	if (!irq_triggered) {
+		report_info("  RTC RIS: %x", readl(&pl031->ris));
+		report_info("  RTC MIS: %x", readl(&pl031->mis));
+		report_info("  RTC IMSC: %x", readl(&pl031->imsc));
+		report_info("  GIC IRQs pending: %08x %08x", readl(gic_ispendr), readl(gic_ispendr + 4));
+	}
+
+	local_irq_disable();
+	return 0;
+}
+
+static void rtc_irq_init(void)
+{
+	gic_enable_defaults();
+
+	switch (gic_version()) {
+	case 2:
+		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;

Quoted 8 lines

arm/pl031.c line 210

Quoted 5 lines

arm/pl031.c new file +5 -0

+		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
+		break;
+	case 3:
+		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+		gic_isenabler = gicv3_sgi_base() + GICD_ISENABLER;

This only points to SGIs and PPIs. How does it work on GICv3? Let me guess: you haven't tested it... ;-)

I tested it, then it failed, then I forgot about it :)

I take it that on GICv3, SPIs live somewhere else?

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Signed-off-by Alexander Graf <[email protected]>

---
 arm/Makefile.common |   1 +
 arm/pl031.c         | 227 ++++++++++++++++++++++++++++++++++++++++++++
 lib/arm/asm/gic.h   |   1 +
 3 files changed, 229 insertions(+)
 create mode 100644 arm/pl031.c

arm/Makefile.common +1 -0

index f0c4b5d..b8988f2 100644
@@ -11,6 +11,7 @@ tests-common += $(TEST_DIR)/pmu.flat
 tests-common += $(TEST_DIR)/gic.flat
 tests-common += $(TEST_DIR)/psci.flat
 tests-common += $(TEST_DIR)/sieve.flat
+tests-common += $(TEST_DIR)/pl031.flat
 
 tests-all = $(tests-common) $(tests)
 all: directories $(tests-all)

arm/pl031.c new file +208 -0

new file mode 100644
index 0000000..a364a1a
@@ -0,0 +1,227 @@
+/*
+ * Verify PL031 functionality
+ *
+ * This test verifies whether the emulated PL031 behaves correctly.
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates.
+ * Author: Alexander Graf <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <asm/processor.h>
+#include <asm/io.h>
+#include <asm/gic.h>
+
+static u32 cntfrq;
+
+#define PL031_BASE 0x09010000
+#define PL031_IRQ 2
+
+struct pl031_regs {
+	uint32_t dr;	/* Data Register */
+	uint32_t mr;	/* Match Register */
+	uint32_t lr;	/* Load Register */
+	union {
+		uint8_t cr;	/* Control Register */
+		uint32_t cr32;
+	};
+	union {
+		uint8_t imsc;	/* Interrupt Mask Set or Clear register */
+		uint32_t imsc32;
+	};
+	union {
+		uint8_t ris;	/* Raw Interrupt Status */
+		uint32_t ris32;
+	};
+	union {
+		uint8_t mis;	/* Masked Interrupt Status */
+		uint32_t mis32;
+	};
+	union {
+		uint8_t icr;	/* Interrupt Clear Register */
+		uint32_t icr32;
+	};
+	uint32_t reserved[1008];
+	uint32_t periph_id[4];
+	uint32_t pcell_id[4];
+};
+
+static struct pl031_regs *pl031 = (void*)PL031_BASE;
+static void *gic_ispendr;
+static void *gic_isenabler;
+static bool irq_triggered;
+
+static int check_id(void)
+{
+	uint32_t id[] = { 0x31, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(id); i++)
+		if (id[i] != readl(&pl031->periph_id[i]))
+			return 1;
+
+	return 0;
+}
+
+static int check_ro(void)
+{
+	uint32_t offs[] = { offsetof(struct pl031_regs, ris),
+			    offsetof(struct pl031_regs, mis),
+			    offsetof(struct pl031_regs, periph_id[0]),
+			    offsetof(struct pl031_regs, periph_id[1]),
+			    offsetof(struct pl031_regs, periph_id[2]),
+			    offsetof(struct pl031_regs, periph_id[3]),
+			    offsetof(struct pl031_regs, pcell_id[0]),
+			    offsetof(struct pl031_regs, pcell_id[1]),
+			    offsetof(struct pl031_regs, pcell_id[2]),
+			    offsetof(struct pl031_regs, pcell_id[3]) };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(offs); i++) {
+		uint32_t before32;
+		uint16_t before16;
+		uint8_t before8;
+		void *addr = (void*)pl031 + offs[i];
+		uint32_t poison = 0xdeadbeefULL;
+
+		before8 = readb(addr);
+		before16 = readw(addr);
+		before32 = readl(addr);
+
+		writeb(poison, addr);
+		writew(poison, addr);
+		writel(poison, addr);
+
+		if (before8 != readb(addr))
+			return 1;
+		if (before16 != readw(addr))
+			return 1;
+		if (before32 != readl(addr))
+			return 1;
+	}
+
+	return 0;
+}
+
+static int check_rtc_freq(void)
+{
+	uint32_t seconds_to_wait = 2;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);
+	uint64_t target_tick = before_tick + (cntfrq * seconds_to_wait);
+
+	/* Wait for 2 seconds */
+	while (read_sysreg(cntpct_el0) < target_tick) ;
+
+	if (readl(&pl031->dr) != before + seconds_to_wait)
+		return 1;
+
+	return 0;
+}
+
+static bool gic_irq_pending(void)
+{
+	return readl(gic_ispendr + 4) & (1 << (SPI(PL031_IRQ) - 32));
+}
+
+static void gic_irq_unmask(void)
+{
+	writel(1 << (SPI(PL031_IRQ) - 32), gic_isenabler + 4);
+}
+
+static void irq_handler(struct pt_regs *regs)
+{
+	u32 irqstat = gic_read_iar();
+	u32 irqnr = gic_iar_irqnr(irqstat);
+
+	if (irqnr != GICC_INT_SPURIOUS)
+		gic_write_eoir(irqstat);
+
+	if (irqnr == SPI(PL031_IRQ)) {
+		report("  RTC RIS == 1", readl(&pl031->ris) == 1);
+		report("  RTC MIS == 1", readl(&pl031->mis) == 1);
+
+		/* Writing any value should clear IRQ status */
+		writel(0x80000000ULL, &pl031->icr);
+
+		report("  RTC RIS == 0", readl(&pl031->ris) == 0);
+		report("  RTC MIS == 0", readl(&pl031->mis) == 0);
+		irq_triggered = true;
+	} else {
+		report_info("Unexpected interrupt: %d\n", irqnr);
+		return;
+	}
+}
+
+static int check_rtc_irq(void)
+{
+	uint32_t seconds_to_wait = 1;
+	uint32_t before = readl(&pl031->dr);
+	uint64_t before_tick = read_sysreg(cntpct_el0);
+	uint64_t target_tick = before_tick + (cntfrq * (seconds_to_wait + 1));
+
+	report_info("Checking IRQ trigger (MR)");
+
+	irq_triggered = false;
+
+	/* Fire IRQ in 1 second */
+	writel(before + seconds_to_wait, &pl031->mr);
+
+	install_irq_handler(EL1H_IRQ, irq_handler);
+
+	/* Wait until 2 seconds are over */
+	while (read_sysreg(cntpct_el0) < target_tick) ;
+
+	report("  RTC IRQ not delivered without mask", !gic_irq_pending());
+
+	/* Mask the IRQ so that it gets delivered */
+	writel(1, &pl031->imsc);
+	report("  RTC IRQ pending now", gic_irq_pending());
+
+	/* Enable retrieval of IRQ */
+	gic_irq_unmask();
+	local_irq_enable();
+
+	report("  IRQ triggered", irq_triggered);
+	report("  RTC IRQ not pending anymore", !gic_irq_pending());
+	if (!irq_triggered) {
+		report_info("  RTC RIS: %x", readl(&pl031->ris));
+		report_info("  RTC MIS: %x", readl(&pl031->mis));
+		report_info("  RTC IMSC: %x", readl(&pl031->imsc));
+		report_info("  GIC IRQs pending: %08x %08x", readl(gic_ispendr), readl(gic_ispendr + 4));
+	}
+
+	local_irq_disable();
+	return 0;
+}
+
+static void rtc_irq_init(void)
+{
+	gic_enable_defaults();
+
+	switch (gic_version()) {
+	case 2:
+		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
+		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
+		break;
+	case 3:

arm/pl031.c new file +9 -0

+
+	report("Periph/PCell IDs match", !check_id());
+	report("R/O fields are R/O", !check_ro());
+	report("RTC ticks at 1HZ", !check_rtc_freq());
+	report("RTC IRQ not pending yet", !gic_irq_pending());
+	check_rtc_irq();
+
+	return report_summary();
+}

lib/arm/asm/gic.h +1 -0

index f6dfb90..1fc10a0 100644
@@ -41,6 +41,7 @@
 #include <asm/gic-v3.h>
 
 #define PPI(irq)			((irq) + 16)
+#define SPI(irq)			((irq) + GIC_FIRST_SPI)
 
 #ifndef __ASSEMBLY__
 #include <asm/cpumask.h>

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Signed-off-by Alexander Graf <[email protected]>

---
 arm/Makefile.common |   1 +
 arm/pl031.c         | 227 ++++++++++++++++++++++++++++++++++++++++++++
 lib/arm/asm/gic.h   |   1 +
 3 files changed, 229 insertions(+)
 create mode 100644 arm/pl031.c

arm/Makefile.common +1 -0

index f0c4b5d..b8988f2 100644
@@ -11,6 +11,7 @@ tests-common += $(TEST_DIR)/pmu.flat
 tests-common += $(TEST_DIR)/gic.flat
 tests-common += $(TEST_DIR)/psci.flat
 tests-common += $(TEST_DIR)/sieve.flat
+tests-common += $(TEST_DIR)/pl031.flat
 
 tests-all = $(tests-common) $(tests)
 all: directories $(tests-all)

arm/pl031.c new file +0 -0

new file mode 100644
index 0000000..a364a1a

Quoted 2 lines

+ * This test verifies whether the emulated PL031 behaves correctly.

^^^^^^^^

While I appreciate the effort and like the fact that this actually triggers an SPI, I wonder if this actually belongs into kvm-unit-tests. After all this just test a device purely emulated in (QEMU) userland, so it's not really KVM related.

What is the general opinion on this? Don't we care about this hair-splitting as long as it helps testing? Do we even want to extend kvm-unit-tests coverage to more emulated devices, for instance virtio?

Quoted 9 lines · Andre Przywara

Quoted 2 lines

+ * This test verifies whether the emulated PL031 behaves correctly.

^^^^^^^^

While I appreciate the effort and like the fact that this actually triggers an SPI, I wonder if this actually belongs into kvm-unit-tests. After all this just test a device purely emulated in (QEMU) userland, so it's not really KVM related.

Quoted 4 lines

What is the general opinion on this? Don't we care about this hair-splitting as long as it helps testing? Do we even want to extend kvm-unit-tests coverage to more emulated devices, for instance virtio?

I agree that it would belong more in qtest, but tests in not exactly the right place is better than no tests.

Quoted 2 lines

I agree that it would belong more in qtest, but tests in not exactly the right place is better than no tests.

The problem with qtest is that it tests QEMU device models from a QEMU internal view.

I am much more interested in the guest visible side of things. If kvmtool wanted to implement a PL031, it should be able to execute the same test that we run against QEMU, no?

If kvm-unit-test is the wrong place for it, we would probably want to have a separate testing framework for guest side unit tests targeting emulated devices.

Given how nice the kvm-unit-test framework is though, I'd rather rename it to "virt-unit-test" than reinvent the wheel :).

Quoted 5 lines

Quoted 2 lines

I agree that it would belong more in qtest, but tests in not exactly the right place is better than no tests.

The problem with qtest is that it tests QEMU device models from a QEMU internal view.

Not really: fundamentally it tests QEMU device models with stimuli that come from another process in the host, rather than code that runs in a guest. It does have hooks into QEMU's internal view (mostly to intercept interrupts and advance the clocks), but the main feature of the protocol is the ability to do memory reads and writes.

Quoted 3 lines

I am much more interested in the guest visible side of things. If kvmtool wanted to implement a PL031, it should be able to execute the same test that we run against QEMU, no?

Well, kvmtool could also implement the qtest protocol; perhaps it should (probably as a different executable that shares the device models with the main kvmtool executable). There would still be issues in reusing code from the QEMU tests, since it has references to QEMU command line options.

Quoted 6 lines

If kvm-unit-test is the wrong place for it, we would probably want to have a separate testing framework for guest side unit tests targeting emulated devices.

Given how nice the kvm-unit-test framework is though, I'd rather rename it to "virt-unit-test" than reinvent the wheel :).

Definitely, or even just "hwtest". :) With my QEMU hat I would prefer the test to be a qtest, but with my kvm-unit-tests hat on I see no reason to reject this test. Sorry if this was not clear.

On Thu, 11 Jul 2019 09:52:42 +0200
Paolo Bonzini <[email protected]> wrote:

Hi,

Quoted 13 lines · Alexander Graf

Quoted 5 lines

Quoted 2 lines

I agree that it would belong more in qtest, but tests in not exactly the right place is better than no tests.

The problem with qtest is that it tests QEMU device models from a QEMU internal view.

Not really: fundamentally it tests QEMU device models with stimuli that come from another process in the host, rather than code that runs in a guest. It does have hooks into QEMU's internal view (mostly to intercept interrupts and advance the clocks), but the main feature of the protocol is the ability to do memory reads and writes.

Quoted 3 lines

Quoted 3 lines

I am much more interested in the guest visible side of things. If kvmtool wanted to implement a PL031, it should be able to execute the same test that we run against QEMU, no?

One of the design goals of kvmtool is to get away with as little emulation as possible, in favour of paravirtualisation (so it's just virtio and not IDE/flash). So a hardware RTC emulation sounds dispensable in this context.

On Thu, 11 Jul 2019 09:52:42 +0200
Paolo Bonzini <[email protected]> wrote:

Hi,

Quoted 13 lines · Alexander Graf

Quoted 5 lines

Quoted 2 lines

I agree that it would belong more in qtest, but tests in not exactly the right place is better than no tests.

The problem with qtest is that it tests QEMU device models from a QEMU internal view.

Not really: fundamentally it tests QEMU device models with stimuli that come from another process in the host, rather than code that runs in a guest. It does have hooks into QEMU's internal view (mostly to intercept interrupts and advance the clocks), but the main feature of the protocol is the ability to do memory reads and writes.

Quoted 3 lines

Quoted 3 lines

I am much more interested in the guest visible side of things. If kvmtool wanted to implement a PL031, it should be able to execute the same test that we run against QEMU, no?

One of the design goals of kvmtool is to get away with as little emulation as possible, in favour of paravirtualisation (so it's just virtio and not IDE/flash). So a hardware RTC emulation sounds dispensable in this context.

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Quoted 5 lines · Alexander Graf

This patch adds a unit test for the PL031 RTC that is used in the virt machine. It just pokes basic functionality. I've mostly written it to familiarize myself with the device, but I suppose having the test around does not hurt, as it also exercises the GIC SPI interrupt path.

Have you tested this against a real hardware pl031? I appreciate that the scaffolding to let you do that is probably pretty painful, but it would be interesting to test, because I'm not really all that confident in the accuracy of QEMU's pl031 model. (Notably there are some places where it absolutely does not work like the real h/w; in some ways it's a bit like "a pl031 that some imaginary firmware has initialized and enabled"...)

Quoted 1 line · Peter Maydell

Have you tested this against a real hardware pl031?

Do you have any pointers to devices I might own that have one?