[PATCH kvm-unit-tests] arm: Add PL031 test

[PATCH kvm-unit-tests] arm: Add PL031 test

[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 <graf@amazon.com>
---
 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

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f0c4b5d..b8988f2 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -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)
diff --git a/arm/pl031.c b/arm/pl031.c
new file mode 100644
index 0000000..a364a1a
--- /dev/null
+++ b/arm/pl031.c
@@ -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 <graf@amazon.com>
+ *
+ * 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:
+		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+		gic_isenabler = gicv3_sgi_base() + GICD_ISENABLER;
+		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();
+}
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index f6dfb90..1fc10a0 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -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>
-- 
2.17.1

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Marc Zyngier]

Hi Alex,

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

On 10/07/2019 14:27, Alexander Graf wrote:
> 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 <graf@amazon.com>
> ---
>  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
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f0c4b5d..b8988f2 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -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)
> diff --git a/arm/pl031.c b/arm/pl031.c
> new file mode 100644
> index 0000000..a364a1a
> --- /dev/null
> +++ b/arm/pl031.c
> @@ -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 <graf@amazon.com>
> + *
> + * 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

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

> +
> +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);

Hmmm. See below.

> +	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 ;-).

> +
> +	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...

> +}
> +
> +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()?

> +	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.

> +
> +	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);

Same problem as above.

> +	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:
> +		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... ;-)

> +		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();
> +}
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index f6dfb90..1fc10a0 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -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>
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Alexandru Elisei]

On 7/10/19 2:27 PM, Alexander Graf wrote:
> 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 <graf@amazon.com>
> ---
>  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
>
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f0c4b5d..b8988f2 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -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)
> diff --git a/arm/pl031.c b/arm/pl031.c
> new file mode 100644
> index 0000000..a364a1a
> --- /dev/null
> +++ b/arm/pl031.c
> @@ -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 <graf@amazon.com>
> + *
> + * 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:
> +		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
> +		gic_isenabler = gicv3_sgi_base() + GICD_ISENABLER;
> +		break;
> +	}
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	cntfrq = get_cntfrq();
> +	rtc_irq_init();

The PL031 device might be generated by QEMU by default, but KVM-unit-tests can
also be run under kvmtool, which doesn't include such a device.

I suggest that you first check that the device is in the fdt, and skip the tests
entirely if it's not present. There's an example in the timer.c test about how
to find a device by using its compatible string. And as Marc suggested, you can
also use the fdt to get the device properties.

Thanks,
Alex
> +
> +	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();
> +}
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index f6dfb90..1fc10a0 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -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>

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Andre Przywara]

On Wed, 10 Jul 2019 15:27:24 +0200
Alexander Graf <graf@amazon.com> wrote:

Hi,

> 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 <graf@amazon.com>
> ---
>  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
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f0c4b5d..b8988f2 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -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)
> diff --git a/arm/pl031.c b/arm/pl031.c
> new file mode 100644
> index 0000000..a364a1a
> --- /dev/null
> +++ b/arm/pl031.c
> @@ -0,0 +1,227 @@
> +/*
> + * Verify PL031 functionality
> + *
> + * 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?

Cheers,
Andre.

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Paolo Bonzini]

On 10/07/19 19:02, Andre Przywara wrote:
>> + * 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?

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

Paolo

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Alexander Graf]

On 10.07.19 19:06, Paolo Bonzini wrote:
> On 10/07/19 19:02, Andre Przywara wrote:
>>> + * 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?
> 
> 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 :).


Alex

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Paolo Bonzini]

On 11/07/19 07:49, Alexander Graf wrote:
>> 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.

> 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.

> 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.

Paolo

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Peter Maydell]

On Wed, 10 Jul 2019 at 14:35, Alexander Graf <graf@amazon.com> wrote:
>
> 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"...)

thanks
-- PMM

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Alexander Graf]

On 11.07.19 10:51, Peter Maydell wrote:
> On Wed, 10 Jul 2019 at 14:35, Alexander Graf <graf@amazon.com> wrote:
>>
>> 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"...)

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

Alex

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Peter Maydell]

On Thu, 11 Jul 2019 at 10:11, Alexander Graf <graf@amazon.com> wrote:
> On 11.07.19 10:51, Peter Maydell wrote:
> > Have you tested this against a real hardware pl031?

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

Heh, fair point. I'd expect to find one in most of the devboards
Arm has shipped over the years, but I dunno if you'll find one
anywhere else.

thanks
-- PMM

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Andre Przywara]

On Thu, 11 Jul 2019 09:52:42 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

Hi,

> On 11/07/19 07:49, Alexander Graf wrote:
> >> 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.
> 
> > 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.
 
> 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.

I had some patches to better abstract kvm-unit-tests from QEMU, basically
by splitting up extra_params into more generic options like memsize and
command_line, then translating them.
Sounds like the time to revive them.

> > 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.

Fair enough, at the moment we have to trigger kvmtool tests manually
anyway. Just wanted to know what the idea is here, which I think you
answered.

Thanks,
Andre.

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Marc Zyngier]

On 11/07/2019 10:42, Andre Przywara wrote:
> On Thu, 11 Jul 2019 09:52:42 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> Hi,
> 
>> On 11/07/19 07:49, Alexander Graf wrote:
>>>> 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.
>>
>>> 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.

Funnily enough, kvmtool does have a RTC emulation (MC146818). The
support code in Linux is so braindead that I gave up on it about 5 years
ago.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Alexander Graf]

On 11.07.19 11:42, Andre Przywara wrote:
> On Thu, 11 Jul 2019 09:52:42 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> Hi,
> 
>> On 11/07/19 07:49, Alexander Graf wrote:
>>>> 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.
>>
>>> 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.

The main reason to have a PL031 exposed to a VM is to make OVMF happy, 
so that it can provide wall clock time runtime services. I suppose that 
sooner or later you may want to run OVMF in kvmtool as well, no?

The alternative to the PL031 here is to do a PV interface, yes. I'm not 
really convinced that that would be any easier though. The PL031 is a 
very trivial device. The only real downside is that it will wrap around 
in 2038.


Alex

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Alexander Graf]

On 10.07.19 16:25, Marc Zyngier wrote:
> Hi Alex,
> 
> I don't know much about pl031, so my comments are pretty general...
> 
> On 10/07/2019 14:27, Alexander Graf wrote:
>> 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 <graf@amazon.com>
>> ---
>>   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
>>
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index f0c4b5d..b8988f2 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -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)
>> diff --git a/arm/pl031.c b/arm/pl031.c
>> new file mode 100644
>> index 0000000..a364a1a
>> --- /dev/null
>> +++ b/arm/pl031.c
>> @@ -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 <graf@amazon.com>
>> + *
>> + * 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
> 
> 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 :).

> 
>> +
>> +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);
> 
> Hmmm. See below.
> 
>> +	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 ;-).

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?

> 
>> +
>> +	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 :).

> 
>> +}
>> +
>> +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 :).

> 
>> +	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);

> 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();
}

> 
>> +
>> +	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);
> 
> Same problem as above.
> 
>> +	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:
>> +		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?


Alex

Re: [PATCH kvm-unit-tests] arm: Add PL031 test

[Marc Zyngier]

On 12/07/2019 09:29, Alexander Graf wrote:
> 
> 
> On 10.07.19 16:25, Marc Zyngier wrote:
>> Hi Alex,
>>
>> I don't know much about pl031, so my comments are pretty general...
>>
>> On 10/07/2019 14:27, Alexander Graf wrote:
>>> 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 <graf@amazon.com>
>>> ---
>>>   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
>>>
>>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>>> index f0c4b5d..b8988f2 100644
>>> --- a/arm/Makefile.common
>>> +++ b/arm/Makefile.common
>>> @@ -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)
>>> diff --git a/arm/pl031.c b/arm/pl031.c
>>> new file mode 100644
>>> index 0000000..a364a1a
>>> --- /dev/null
>>> +++ b/arm/pl031.c
>>> @@ -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 <graf@amazon.com>
>>> + *
>>> + * 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
>>
>> 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 :).
> 
>>
>>> +
>>> +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);
>>
>> Hmmm. See below.
>>
>>> +	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 ;-).
> 
> 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?

Well, on a highly speculative core, you should be able to get about any
timing you can imagine if you don't have actual synchronization. At
least the ISB should definitely be there.

> 
>>
>>> +
>>> +	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 :).
> 
>>
>>> +}
>>> +
>>> +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 :).

Well, I haven't read the GIC code in kvm-unit-tests, hence my questions...

> 
>>
>>> +	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);

OK then.

>> 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();
> }

Good.

> 
>>
>>> +
>>> +	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);
>>
>> Same problem as above.
>>
>>> +	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:
>>> +		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?

They live in the distributor, just like on GICv2. Only the cpu-private
interrupts are located in the redistributors.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...