[PATCH kvm-unit-tests 0/3] Add physical timer test

[PATCH kvm-unit-tests 0/3] Add physical timer test

[Christoffer Dall]

Add a test for the vtimer. I've tested on
 accel=tcg
 accel=kvm                     : on seattle, and mustang
 accel=kvm,kernel-irqchip=off  : on mustang

I first fix two issues I had running the basic timer test on APM mustang
on using TCG.  I wonder why the vtimer tests worked using TCG for Drew,
since they didn't work for me, and I don't see how they would have
without patch 1.

Then I introduce a test for the physical timer.  If you run the ptimer
test on a kernel before support for the physical timers was added you
get a nice error message plus some spamming in your kernel log.


Christoffer Dall (3):
  arm64: timer: Fix vtimer interrupt test
  arm64: timer: Fix test on APM X-Gene
  arm64: timer: Add support for phys timer testing

 arm/timer.c       | 205 +++++++++++++++++++++++++++++++++++++++++++++---------
 arm/unittests.cfg |   9 ++-
 2 files changed, 182 insertions(+), 32 deletions(-)

-- 
2.9.0

[PATCH kvm-unit-tests 1/3] arm64: timer: Fix vtimer interrupt test

[Christoffer Dall]

The timer irq_handler is supposed to mask the timer signal, but unfortunately
also disables the timer at the same time, even though we loop and wait on
ISTATUS to become set.

According to the ARM ARM, "When the value of the ENABLE bit is 0, the
ISTATUS field is UNKNOWN."  This test happens to work on AMD Seattle, but
doesn't work on Mustang or on QEMU with TCG.

Fix the problem by preserving the enable bit in the irq handler.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arm/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/timer.c b/arm/timer.c
index 89f4c94..02d9e0a 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -28,7 +28,7 @@ static void irq_handler(struct pt_regs *regs)
 		gic_write_eoir(irqstat);
 
 	if (irqnr == PPI(vtimer_irq)) {
-		write_sysreg(CNTV_CTL_IMASK, cntv_ctl_el0);
+		write_sysreg(CNTV_CTL_IMASK | CNTV_CTL_ENABLE, cntv_ctl_el0);
 		++vtimer_irq_received;
 	}
 }
-- 
2.9.0

[PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Christoffer Dall]

When running the vtimer test on an APM X-Gene, setting the timer value
to (2^64 - 1) apparently results in the timer always firing, even
thought the counter is mich lower than the cval.

Since the idea of the code is to set everything up and schedule the
timer for some time very far in the future, take a pragmatic approach
and just add 10s worth of delay instead.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arm/timer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 02d9e0a..77d257c 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -70,10 +70,14 @@ static bool test_cval_10msec(void)
 
 static void test_vtimer(void)
 {
+	u64 now = read_sysreg(cntvct_el0);
+	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
+	u64 later = now + time_10s;
+
 	report_prefix_push("vtimer-busy-loop");
 
-	/* Enable the timer */
-	write_sysreg(~0, cntv_cval_el0);
+	/* Enable the timer, but schedule it for much later*/
+	write_sysreg(later, cntv_cval_el0);
 	isb();
 	write_sysreg(CNTV_CTL_ENABLE, cntv_ctl_el0);
 
-- 
2.9.0

[PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Christoffer Dall]

Rearrange the code to be able to reuse as much as posible and add
support for testing the physical timer as well.

Also change the default unittests configuration to run a separate vtimer
and ptimer test so that older kernels without ptimer support just show a
failure.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arm/timer.c       | 199 ++++++++++++++++++++++++++++++++++++++++++++++--------
 arm/unittests.cfg |   9 ++-
 2 files changed, 177 insertions(+), 31 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 77d257c..33dfc6f 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -11,34 +11,119 @@
 #include <asm/gic.h>
 #include <asm/io.h>
 
-#define CNTV_CTL_ENABLE  (1 << 0)
-#define CNTV_CTL_IMASK   (1 << 1)
-#define CNTV_CTL_ISTATUS (1 << 2)
+#define ARCH_TIMER_CTL_ENABLE  (1 << 0)
+#define ARCH_TIMER_CTL_IMASK   (1 << 1)
+#define ARCH_TIMER_CTL_ISTATUS (1 << 2)
 
-static u32 vtimer_irq, vtimer_irq_flags;
 static void *gic_ispendr;
-static bool vtimer_irq_received;
+
+static u64 read_vtimer_counter(void)
+{
+	return read_sysreg(cntvct_el0);
+}
+
+static u64 read_vtimer_cval(void)
+{
+	return read_sysreg(cntv_cval_el0);
+}
+
+static void write_vtimer_cval(u64 val)
+{
+	write_sysreg(val, cntv_cval_el0);
+}
+
+static u64 read_vtimer_ctl(void)
+{
+	return read_sysreg(cntv_ctl_el0);
+}
+
+static void write_vtimer_ctl(u64 val)
+{
+	write_sysreg(val, cntv_ctl_el0);
+}
+
+static u64 read_ptimer_counter(void)
+{
+	return read_sysreg(cntpct_el0);
+}
+
+static u64 read_ptimer_cval(void)
+{
+	return read_sysreg(cntp_cval_el0);
+}
+
+static void write_ptimer_cval(u64 val)
+{
+	write_sysreg(val, cntp_cval_el0);
+}
+
+static u64 read_ptimer_ctl(void)
+{
+	return read_sysreg(cntp_ctl_el0);
+}
+
+static void write_ptimer_ctl(u64 val)
+{
+	write_sysreg(val, cntp_ctl_el0);
+}
+
+struct timer_info {
+	u32 irq;
+	u32 irq_flags;
+	bool irq_received;
+	u64 (*read_counter)(void);
+	u64 (*read_cval)(void);
+	void (*write_cval)(u64);
+	u64 (*read_ctl)(void);
+	void (*write_ctl)(u64);
+};
+
+static struct timer_info vtimer_info = {
+	.irq_received = false,
+	.read_counter = read_vtimer_counter,
+	.read_cval = read_vtimer_cval,
+	.write_cval = write_vtimer_cval,
+	.read_ctl = read_vtimer_ctl,
+	.write_ctl = write_vtimer_ctl,
+};
+
+static struct timer_info ptimer_info = {
+	.irq_received = false,
+	.read_counter = read_ptimer_counter,
+	.read_cval = read_ptimer_cval,
+	.write_cval = write_ptimer_cval,
+	.read_ctl = read_ptimer_ctl,
+	.write_ctl = write_ptimer_ctl,
+};
 
 static void irq_handler(struct pt_regs *regs)
 {
+	struct timer_info *info;
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
 
 	if (irqnr != GICC_INT_SPURIOUS)
 		gic_write_eoir(irqstat);
 
-	if (irqnr == PPI(vtimer_irq)) {
-		write_sysreg(CNTV_CTL_IMASK | CNTV_CTL_ENABLE, cntv_ctl_el0);
-		++vtimer_irq_received;
+	if (irqnr == PPI(vtimer_info.irq)) {
+		info = &vtimer_info;
+	} else if (irqnr == PPI(ptimer_info.irq)) {
+		info = &ptimer_info;
+	} else {
+		report_info("Unexpected interrupt: %d\n", irqnr);
+		return;
 	}
+
+	info->write_ctl(ARCH_TIMER_CTL_IMASK | ARCH_TIMER_CTL_ENABLE);
+	info->irq_received = true;
 }
 
-static bool gic_vtimer_pending(void)
+static bool gic_timer_pending(struct timer_info *info)
 {
-	return readl(gic_ispendr) & (1 << PPI(vtimer_irq));
+	return readl(gic_ispendr) & (1 << PPI(info->irq));
 }
 
-static bool test_cval_10msec(void)
+static bool test_cval_10msec(struct timer_info *info)
 {
 	u64 time_10ms = read_sysreg(cntfrq_el0) / 100;
 	u64 time_1us = time_10ms / 10000;
@@ -46,15 +131,15 @@ static bool test_cval_10msec(void)
 	s64 difference;
 
 	/* Program timer to fire in 10 ms */
-	before_timer = read_sysreg(cntvct_el0);
-	write_sysreg(before_timer + time_10ms, cntv_cval_el0);
+	before_timer = info->read_counter();
+	info->write_cval(before_timer + time_10ms);
 
 	/* Wait for the timer to fire */
-	while (!(read_sysreg(cntv_ctl_el0) & CNTV_CTL_ISTATUS))
+	while (!(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS))
 		;
 
 	/* It fired, check how long it took */
-	after_timer = read_sysreg(cntvct_el0);
+	after_timer = info->read_counter();
 	difference = after_timer - (before_timer + time_10ms);
 
 	report_info("After timer: 0x%016lx", after_timer);
@@ -62,32 +147,43 @@ static bool test_cval_10msec(void)
 	report_info("Difference : %ld us", difference / time_1us);
 
 	if (difference < 0) {
-		printf("CNTV_CTL_EL0.ISTATUS set too early\n");
+		printf("ISTATUS set too early\n");
 		return false;
 	}
 	return difference < time_10ms;
 }
 
-static void test_vtimer(void)
+static void test_timer(struct timer_info *info)
 {
 	u64 now = read_sysreg(cntvct_el0);
 	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
 	u64 later = now + time_10s;
 
-	report_prefix_push("vtimer-busy-loop");
 
 	/* Enable the timer, but schedule it for much later*/
-	write_sysreg(later, cntv_cval_el0);
+	info->write_cval(later);
 	isb();
-	write_sysreg(CNTV_CTL_ENABLE, cntv_ctl_el0);
+	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
 
-	report("not pending before", !gic_vtimer_pending());
-	report("latency within 10 ms", test_cval_10msec());
-	report("interrupt received", vtimer_irq_received);
+	report("not pending before", !gic_timer_pending(info));
+	report("latency within 10 ms", test_cval_10msec(info));
+	report("interrupt received", info->irq_received);
 
 	/* Disable the timer again */
-	write_sysreg(0, cntv_ctl_el0);
+	info->write_ctl(0);
+}
+
+static void test_vtimer(void)
+{
+	report_prefix_push("vtimer-busy-loop");
+	test_timer(&vtimer_info);
+	report_prefix_pop();
+}
 
+static void test_ptimer(void)
+{
+	report_prefix_push("ptimer-busy-loop");
+	test_timer(&ptimer_info);
 	report_prefix_pop();
 }
 
@@ -102,20 +198,26 @@ static void test_init(void)
 	assert(node >= 0);
 	prop = fdt_get_property(fdt, node, "interrupts", &len);
 	assert(prop && len == (4 * 3 * sizeof(u32)));
+
 	data = (u32 *)prop->data;
+	assert(fdt32_to_cpu(data[3]) == 1);
+	ptimer_info.irq = fdt32_to_cpu(data[4]);
+	ptimer_info.irq_flags = fdt32_to_cpu(data[5]);
 	assert(fdt32_to_cpu(data[6]) == 1);
-	vtimer_irq = fdt32_to_cpu(data[7]);
-	vtimer_irq_flags = fdt32_to_cpu(data[8]);
+	vtimer_info.irq = fdt32_to_cpu(data[7]);
+	vtimer_info.irq_flags = fdt32_to_cpu(data[8]);
 
 	gic_enable_defaults();
 
 	switch (gic_version()) {
 	case 2:
-		writel(1 << PPI(vtimer_irq), gicv2_dist_base() + GICD_ISENABLER + 0);
+		writel(1 << PPI(vtimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
+		writel(1 << PPI(ptimer_info.irq), gicv2_dist_base() + GICD_ISENABLER + 0);
 		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
 		break;
 	case 3:
-		writel(1 << PPI(vtimer_irq), gicv3_sgi_base() + GICR_ISENABLER0);
+		writel(1 << PPI(vtimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
+		writel(1 << PPI(ptimer_info.irq), gicv3_sgi_base() + GICR_ISENABLER0);
 		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
 		break;
 	}
@@ -124,15 +226,52 @@ static void test_init(void)
 	local_irq_enable();
 }
 
-int main(void)
+static void print_vtimer_info(void)
 {
 	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
 	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
 	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
 	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
+}
+
+static void print_ptimer_info(void)
+{
+	printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
+	printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
+	printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
+}
+
+
+int main(int argc, char **argv)
+{
+	bool run_ptimer_test = false;
+	bool run_vtimer_test = false;
+
+	/* Check if we should also check the physical timer */
+	if (argc > 1) {
+		if (strcmp(argv[1], "vtimer") == 0) {
+			run_vtimer_test = true;
+		} else if (strcmp(argv[1], "ptimer") == 0) {
+			run_ptimer_test = true;
+		} else {
+			report_abort("Unknown option '%s'", argv[1]);
+		}
+	} else {
+		run_vtimer_test = true;
+	}
+
+	if (run_vtimer_test)
+		print_vtimer_info();
+	else if (run_ptimer_test)
+		print_ptimer_info();
 
 	test_init();
-	test_vtimer();
+
+	if (run_vtimer_test)
+		test_vtimer();
+	else if (run_ptimer_test)
+		test_ptimer();
+
 
 	return report_summary();
 }
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index bdfedf8..d55e52e 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -111,7 +111,14 @@ smp = $MAX_SMP
 groups = psci
 
 # Timer tests
-[timer]
+[vtimer]
 file = timer.flat
+extra_params = -append 'vtimer'
+groups = timer
+timeout = 2s
+
+[ptimer]
+file = timer.flat
+extra_params = -append 'ptimer'
 groups = timer
 timeout = 2s
-- 
2.9.0

Re: [PATCH kvm-unit-tests 1/3] arm64: timer: Fix vtimer interrupt test

[Marc Zyngier]

On 13/07/17 20:20, Christoffer Dall wrote:
> The timer irq_handler is supposed to mask the timer signal, but unfortunately
> also disables the timer at the same time, even though we loop and wait on
> ISTATUS to become set.
> 
> According to the ARM ARM, "When the value of the ENABLE bit is 0, the
> ISTATUS field is UNKNOWN."  This test happens to work on AMD Seattle, but
> doesn't work on Mustang or on QEMU with TCG.

Note that this wording is a tightening of what was written in previous
of the ARMv8 ARM. Version DDI0874B.a has the above, while DDI0487A.h
doesn't, neither has the ARMv7 version.

This may explain why this was initially written this way.

Thanks,

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

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Marc Zyngier]

On 13/07/17 20:20, Christoffer Dall wrote:
> When running the vtimer test on an APM X-Gene, setting the timer value
> to (2^64 - 1) apparently results in the timer always firing, even
> thought the counter is mich lower than the cval.

Note that the system counter is only guaranteed to be at least 56 bit
wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
has the minimum. This could explain why setting the comparator to a
value greater than (2^56 - 1) leads to a firing timer (the comparator
appears to be in the past).

Thanks,

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

Re: [PATCH kvm-unit-tests 1/3] arm64: timer: Fix vtimer interrupt test

[Christoffer Dall]

On Fri, Jul 14, 2017 at 08:55:32AM +0100, Marc Zyngier wrote:
> On 13/07/17 20:20, Christoffer Dall wrote:
> > The timer irq_handler is supposed to mask the timer signal, but unfortunately
> > also disables the timer at the same time, even though we loop and wait on
> > ISTATUS to become set.
> > 
> > According to the ARM ARM, "When the value of the ENABLE bit is 0, the
> > ISTATUS field is UNKNOWN."  This test happens to work on AMD Seattle, but
> > doesn't work on Mustang or on QEMU with TCG.
> 
> Note that this wording is a tightening of what was written in previous
> of the ARMv8 ARM. Version DDI0874B.a has the above, while DDI0487A.h
> doesn't, neither has the ARMv7 version.
> 
> This may explain why this was initially written this way.
> 

ok, but even so, this patch should test things in a way that should work
on all systems, right?

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Christoffer Dall]

On Fri, Jul 14, 2017 at 09:04:10AM +0100, Marc Zyngier wrote:
> On 13/07/17 20:20, Christoffer Dall wrote:
> > When running the vtimer test on an APM X-Gene, setting the timer value
> > to (2^64 - 1) apparently results in the timer always firing, even
> > thought the counter is mich lower than the cval.
> 
> Note that the system counter is only guaranteed to be at least 56 bit
> wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
> has the minimum. This could explain why setting the comparator to a
> value greater than (2^56 - 1) leads to a firing timer (the comparator
> appears to be in the past).

Thanks for pointing that out, that makes good sense.  So then we should
definitely fix the test.

We could either set it to 2^56 - 1 instead, or just keep the 10s as used
in this patch, because the whole test times out after 2s anyway.

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 1/3] arm64: timer: Fix vtimer interrupt test

[Marc Zyngier]

On 14/07/17 16:43, Christoffer Dall wrote:
> On Fri, Jul 14, 2017 at 08:55:32AM +0100, Marc Zyngier wrote:
>> On 13/07/17 20:20, Christoffer Dall wrote:
>>> The timer irq_handler is supposed to mask the timer signal, but unfortunately
>>> also disables the timer at the same time, even though we loop and wait on
>>> ISTATUS to become set.
>>>
>>> According to the ARM ARM, "When the value of the ENABLE bit is 0, the
>>> ISTATUS field is UNKNOWN."  This test happens to work on AMD Seattle, but
>>> doesn't work on Mustang or on QEMU with TCG.
>>
>> Note that this wording is a tightening of what was written in previous
>> of the ARMv8 ARM. Version DDI0874B.a has the above, while DDI0487A.h
>> doesn't, neither has the ARMv7 version.
>>
>> This may explain why this was initially written this way.
>>
> 
> ok, but even so, this patch should test things in a way that should work
> on all systems, right?

Oh, definitely. I'm not arguing that your fix is wrong, quite the
opposite. It is just that the architecture got stricter over time, and
SW should certainly take the pessimistic approach that is something can
be UNKNOWN, it will.

Thanks,

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

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Andrew Jones]

On Fri, Jul 14, 2017 at 08:45:12AM -0700, Christoffer Dall wrote:
> On Fri, Jul 14, 2017 at 09:04:10AM +0100, Marc Zyngier wrote:
> > On 13/07/17 20:20, Christoffer Dall wrote:
> > > When running the vtimer test on an APM X-Gene, setting the timer value
> > > to (2^64 - 1) apparently results in the timer always firing, even
> > > thought the counter is mich lower than the cval.
> > 
> > Note that the system counter is only guaranteed to be at least 56 bit
> > wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
> > has the minimum. This could explain why setting the comparator to a
> > value greater than (2^56 - 1) leads to a firing timer (the comparator
> > appears to be in the past).

Ah, that explains why when I tried setting it to ~0 on my mustang, and
then reading it back, it was always 2^56 - 1 instead. However my mustang
still also requires me to clear bit 31, otherwise the vcpu hangs.

> 
> Thanks for pointing that out, that makes good sense.  So then we should
> definitely fix the test.
> 
> We could either set it to 2^56 - 1 instead, or just keep the 10s as used
> in this patch, because the whole test times out after 2s anyway.

With the 10s version the test runs and passes on my mustang, so on one
hand I prefer it. OTOH, testing to the spec, by using 2^56 - 1, seems
more correct and allows one to find issues like the one I have on my
mustang, i.e. a vcpu hang when bit 31 isn't clear.

I guess I lean more towards testings to the spec, but not enough to
ask for a v2 of the patch. It's up to you.

Thanks,
drew

Re: [PATCH kvm-unit-tests 0/3] Add physical timer test

[Andrew Jones]

On Thu, Jul 13, 2017 at 09:20:06PM +0200, Christoffer Dall wrote:
> Add a test for the vtimer. I've tested on
>  accel=tcg
>  accel=kvm                     : on seattle, and mustang
>  accel=kvm,kernel-irqchip=off  : on mustang
> 
> I first fix two issues I had running the basic timer test on APM mustang
> on using TCG.  I wonder why the vtimer tests worked using TCG for Drew,
> since they didn't work for me, and I don't see how they would have
> without patch 1.

That's weird. I just tested again with a latest qemu master pull and it
still works for me without patch 1. However, as you and Marc discussed,
it's the right thing to do with regards to the spec.

> 
> Then I introduce a test for the physical timer.  If you run the ptimer
> test on a kernel before support for the physical timers was added you
> get a nice error message plus some spamming in your kernel log.

Thanks for contributing to kvm-unit-tests!

drew

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Christoffer Dall]

On Tue, Jul 18, 2017 at 12:05:03PM +0200, Andrew Jones wrote:
> On Fri, Jul 14, 2017 at 08:45:12AM -0700, Christoffer Dall wrote:
> > On Fri, Jul 14, 2017 at 09:04:10AM +0100, Marc Zyngier wrote:
> > > On 13/07/17 20:20, Christoffer Dall wrote:
> > > > When running the vtimer test on an APM X-Gene, setting the timer value
> > > > to (2^64 - 1) apparently results in the timer always firing, even
> > > > thought the counter is mich lower than the cval.
> > > 
> > > Note that the system counter is only guaranteed to be at least 56 bit
> > > wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
> > > has the minimum. This could explain why setting the comparator to a
> > > value greater than (2^56 - 1) leads to a firing timer (the comparator
> > > appears to be in the past).
> 
> Ah, that explains why when I tried setting it to ~0 on my mustang, and
> then reading it back, it was always 2^56 - 1 instead. However my mustang
> still also requires me to clear bit 31, otherwise the vcpu hangs.
> 
> > 
> > Thanks for pointing that out, that makes good sense.  So then we should
> > definitely fix the test.
> > 
> > We could either set it to 2^56 - 1 instead, or just keep the 10s as used
> > in this patch, because the whole test times out after 2s anyway.
> 
> With the 10s version the test runs and passes on my mustang, so on one
> hand I prefer it. OTOH, testing to the spec, by using 2^56 - 1, seems
> more correct and allows one to find issues like the one I have on my
> mustang, i.e. a vcpu hang when bit 31 isn't clear.
> 
> I guess I lean more towards testings to the spec, but not enough to
> ask for a v2 of the patch. It's up to you.

I think we should have something that tests KVM on the platforms we have
and that are available for people's use.  I don't think we should verify
the architecture as much.  People use the m400 (basically
Mustang) in CloudLab, for example, which is we I keep caring about that.

I think this test was designed to test "if I program a timer to some
time in the future it shouldn't fire right away", which is still what
we test with this patch.

If we want to add a "the platform provides a timer with 56 valid bits in
the counter and compare register", then I think it should be a separate
test, and the the user can see that "basic stuff works", "architecture
compliance not so much" and shrug accordingly.

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 0/3] Add physical timer test

[Christoffer Dall]

On Tue, Jul 18, 2017 at 12:17:07PM +0200, Andrew Jones wrote:
> On Thu, Jul 13, 2017 at 09:20:06PM +0200, Christoffer Dall wrote:
> > Add a test for the vtimer. I've tested on
> >  accel=tcg
> >  accel=kvm                     : on seattle, and mustang
> >  accel=kvm,kernel-irqchip=off  : on mustang
> > 
> > I first fix two issues I had running the basic timer test on APM mustang
> > on using TCG.  I wonder why the vtimer tests worked using TCG for Drew,
> > since they didn't work for me, and I don't see how they would have
> > without patch 1.
> 
> That's weird. I just tested again with a latest qemu master pull and it
> still works for me without patch 1. However, as you and Marc discussed,
> it's the right thing to do with regards to the spec.
> 

What is the command line you use to test this?

By looking at the code in QEMU, I see this:

target/arm/helper.c:gt_recalc_timer()

    if (gt->ctl & 1) {
        ...
    } else {
        gt->ctl &= ~4;
        qemu_set_irq(cpu->gt_timer_outputs[timeridx], 0);
        timer_del(cpu->gt_timer[timeridx]);
        trace_arm_gt_recalc_disabled(timeridx);
    }

So bits 1 and 2 (IMASK and ISTATUS, respectively) are cleared.

I instrumented the code and verified this.

Confusing.

> > 
> > Then I introduce a test for the physical timer.  If you run the ptimer
> > test on a kernel before support for the physical timers was added you
> > get a nice error message plus some spamming in your kernel log.
> 
> Thanks for contributing to kvm-unit-tests!
> 

Thanks for maintaining kvm-unit-tests!

It has actually been incredibly useful for developing my timer patches.

-Christoffer

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Andrew Jones]

On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> Rearrange the code to be able to reuse as much as posible and add
> support for testing the physical timer as well.
> 
> Also change the default unittests configuration to run a separate vtimer
> and ptimer test so that older kernels without ptimer support just show a
> failure.

We could run tests for both the ptimer and vtimer in a single execution,
rather than splitting them and requiring the input, because the read of
cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
applying the errata framework we can ensure that if we expect the read
to work, i.e. the host kernel is recent enough, then, if we still get
an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
add on patch that makes the conversion. Let me know what you think.

Thanks,
drew

diff --git a/arm/timer.c b/arm/timer.c
index 33dfc6facc190..d0ba1e9a3bafa 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -7,6 +7,7 @@
  */
 #include <libcflat.h>
 #include <devicetree.h>
+#include <errata.h>
 #include <asm/processor.h>
 #include <asm/gic.h>
 #include <asm/io.h>
@@ -16,6 +17,27 @@
 #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
 
 static void *gic_ispendr;
+static bool ptimer_unsupported;
+
+static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
+{
+	ptimer_unsupported = true;
+	regs->pc += 4;
+}
+
+static void set_ptimer_unsupported(void)
+{
+	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
+	read_sysreg(cntp_ctl_el0);
+	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, NULL);
+
+	if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
+		report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
+	} else if (ptimer_unsupported) {
+		report("ptimer: read CNTP_CTL_EL0", false);
+		report_info("ptimer: skipping remaining tests");
+	}
+}
 
 static u64 read_vtimer_counter(void)
 {
@@ -159,7 +181,6 @@ static void test_timer(struct timer_info *info)
 	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
 	u64 later = now + time_10s;
 
-
 	/* Enable the timer, but schedule it for much later*/
 	info->write_cval(later);
 	isb();
@@ -182,6 +203,9 @@ static void test_vtimer(void)
 
 static void test_ptimer(void)
 {
+	if (ptimer_unsupported)
+		return;
+
 	report_prefix_push("ptimer-busy-loop");
 	test_timer(&ptimer_info);
 	report_prefix_pop();
@@ -226,52 +250,27 @@ static void test_init(void)
 	local_irq_enable();
 }
 
-static void print_vtimer_info(void)
+static void print_timer_info(void)
 {
 	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
+
+	if (!ptimer_unsupported) {
+		printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
+		printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
+		printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
+	}
+
 	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
 	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
 	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
 }
 
-static void print_ptimer_info(void)
-{
-	printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
-	printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
-	printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
-}
-
-
 int main(int argc, char **argv)
 {
-	bool run_ptimer_test = false;
-	bool run_vtimer_test = false;
-
-	/* Check if we should also check the physical timer */
-	if (argc > 1) {
-		if (strcmp(argv[1], "vtimer") == 0) {
-			run_vtimer_test = true;
-		} else if (strcmp(argv[1], "ptimer") == 0) {
-			run_ptimer_test = true;
-		} else {
-			report_abort("Unknown option '%s'", argv[1]);
-		}
-	} else {
-		run_vtimer_test = true;
-	}
-
-	if (run_vtimer_test)
-		print_vtimer_info();
-	else if (run_ptimer_test)
-		print_ptimer_info();
-
+	set_ptimer_unsupported();
+	print_timer_info();
 	test_init();
-
-	if (run_vtimer_test)
-		test_vtimer();
-	else if (run_ptimer_test)
-		test_ptimer();
-
-
+	test_ptimer();
+	test_vtimer();
 	return report_summary();
 }
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index d55e52e1a4c4f..bdfedf86b01cb 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -111,14 +111,7 @@ smp = $MAX_SMP
 groups = psci
 
 # Timer tests
-[vtimer]
+[timer]
 file = timer.flat
-extra_params = -append 'vtimer'
-groups = timer
-timeout = 2s
-
-[ptimer]
-file = timer.flat
-extra_params = -append 'ptimer'
 groups = timer
 timeout = 2s

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Andrew Jones]

On Tue, Jul 18, 2017 at 12:35:12PM +0200, Christoffer Dall wrote:
> On Tue, Jul 18, 2017 at 12:05:03PM +0200, Andrew Jones wrote:
> > On Fri, Jul 14, 2017 at 08:45:12AM -0700, Christoffer Dall wrote:
> > > On Fri, Jul 14, 2017 at 09:04:10AM +0100, Marc Zyngier wrote:
> > > > On 13/07/17 20:20, Christoffer Dall wrote:
> > > > > When running the vtimer test on an APM X-Gene, setting the timer value
> > > > > to (2^64 - 1) apparently results in the timer always firing, even
> > > > > thought the counter is mich lower than the cval.
> > > > 
> > > > Note that the system counter is only guaranteed to be at least 56 bit
> > > > wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
> > > > has the minimum. This could explain why setting the comparator to a
> > > > value greater than (2^56 - 1) leads to a firing timer (the comparator
> > > > appears to be in the past).
> > 
> > Ah, that explains why when I tried setting it to ~0 on my mustang, and
> > then reading it back, it was always 2^56 - 1 instead. However my mustang
> > still also requires me to clear bit 31, otherwise the vcpu hangs.
> > 
> > > 
> > > Thanks for pointing that out, that makes good sense.  So then we should
> > > definitely fix the test.
> > > 
> > > We could either set it to 2^56 - 1 instead, or just keep the 10s as used
> > > in this patch, because the whole test times out after 2s anyway.
> > 
> > With the 10s version the test runs and passes on my mustang, so on one
> > hand I prefer it. OTOH, testing to the spec, by using 2^56 - 1, seems
> > more correct and allows one to find issues like the one I have on my
> > mustang, i.e. a vcpu hang when bit 31 isn't clear.
> > 
> > I guess I lean more towards testings to the spec, but not enough to
> > ask for a v2 of the patch. It's up to you.
> 
> I think we should have something that tests KVM on the platforms we have
> and that are available for people's use.  I don't think we should verify
> the architecture as much.  People use the m400 (basically
> Mustang) in CloudLab, for example, which is we I keep caring about that.
> 
> I think this test was designed to test "if I program a timer to some
> time in the future it shouldn't fire right away", which is still what
> we test with this patch.
> 
> If we want to add a "the platform provides a timer with 56 valid bits in
> the counter and compare register", then I think it should be a separate
> test, and the the user can see that "basic stuff works", "architecture
> compliance not so much" and shrug accordingly.

Two separate tests sounds good to me. Although, if the value of 'now' is
large enough that now + 10s will set bit 31, then a mustang run (at least
mine) will fail - and most likely cause a lot of confusion, since it
normally does not. We should probably attempt to address that known issue
in some way as well.

Thanks,
drew

Re: [PATCH kvm-unit-tests 0/3] Add physical timer test

[Andrew Jones]

On Tue, Jul 18, 2017 at 12:42:26PM +0200, Christoffer Dall wrote:
> On Tue, Jul 18, 2017 at 12:17:07PM +0200, Andrew Jones wrote:
> > On Thu, Jul 13, 2017 at 09:20:06PM +0200, Christoffer Dall wrote:
> > > Add a test for the vtimer. I've tested on
> > >  accel=tcg
> > >  accel=kvm                     : on seattle, and mustang
> > >  accel=kvm,kernel-irqchip=off  : on mustang
> > > 
> > > I first fix two issues I had running the basic timer test on APM mustang
> > > on using TCG.  I wonder why the vtimer tests worked using TCG for Drew,
> > > since they didn't work for me, and I don't see how they would have
> > > without patch 1.
> > 
> > That's weird. I just tested again with a latest qemu master pull and it
> > still works for me without patch 1. However, as you and Marc discussed,
> > it's the right thing to do with regards to the spec.
> > 
> 
> What is the command line you use to test this?

The one generated by the run script

$ cat logs/timer.log 
timeout -k 1s --foreground 2s ../build/q/aarch64-softmmu/qemu-system-aarch64 -nodefaults -machine virt,accel=tcg -cpu cortex-a57 -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/timer.flat -smp 1 # -initrd /tmp/tmp.2Z4KzFEbAs
CNTFRQ_EL0   : 0x0000000003b9aca0
CNTVCT_EL0   : 0x00000000000aa949
CNTV_CTL_EL0 : 0x0000000000000000
CNTV_CVAL_EL0: 0x0000000000000000
PASS: vtimer-busy-loop: not pending before
INFO: vtimer-busy-loop: After timer: 0x0000000000167bd1
INFO: vtimer-busy-loop: Expected   : 0x0000000000163ae6
INFO: vtimer-busy-loop: Difference : 268 us
PASS: vtimer-busy-loop: latency within 10 ms
PASS: vtimer-busy-loop: interrupt received
SUMMARY: 3 tests

> 
> By looking at the code in QEMU, I see this:
> 
> target/arm/helper.c:gt_recalc_timer()
> 
>     if (gt->ctl & 1) {
>         ...
>     } else {
>         gt->ctl &= ~4;
>         qemu_set_irq(cpu->gt_timer_outputs[timeridx], 0);
>         timer_del(cpu->gt_timer[timeridx]);
>         trace_arm_gt_recalc_disabled(timeridx);
>     }
> 
> So bits 1 and 2 (IMASK and ISTATUS, respectively) are cleared.
> 
> I instrumented the code and verified this.
> 
> Confusing.

Indeed.

drew

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Christoffer Dall]

On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
> On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> > Rearrange the code to be able to reuse as much as posible and add
> > support for testing the physical timer as well.
> > 
> > Also change the default unittests configuration to run a separate vtimer
> > and ptimer test so that older kernels without ptimer support just show a
> > failure.
> 
> We could run tests for both the ptimer and vtimer in a single execution,
> rather than splitting them and requiring the input, because the read of
> cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
> applying the errata framework we can ensure that if we expect the read
> to work, i.e. the host kernel is recent enough, then, if we still get
> an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
> add on patch that makes the conversion. Let me know what you think.

The problem with this patch, is that we then report SKIP instead of
FAIL, when we regress the kernel and actually break physical counter
access (which I did while developing my series).

I think something like this should be discovered by way of capabilities
or hardcoding a kernel version.

Does kvm-unit-tests generally care about not reporting FAIL with earlier
kernel versions?

Thanks,
-Christoffer

> 
> diff --git a/arm/timer.c b/arm/timer.c
> index 33dfc6facc190..d0ba1e9a3bafa 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -7,6 +7,7 @@
>   */
>  #include <libcflat.h>
>  #include <devicetree.h>
> +#include <errata.h>
>  #include <asm/processor.h>
>  #include <asm/gic.h>
>  #include <asm/io.h>
> @@ -16,6 +17,27 @@
>  #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
>  
>  static void *gic_ispendr;
> +static bool ptimer_unsupported;
> +
> +static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
> +{
> +	ptimer_unsupported = true;
> +	regs->pc += 4;
> +}
> +
> +static void set_ptimer_unsupported(void)
> +{
> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
> +	read_sysreg(cntp_ctl_el0);
> +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, NULL);
> +
> +	if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
> +		report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
> +	} else if (ptimer_unsupported) {
> +		report("ptimer: read CNTP_CTL_EL0", false);
> +		report_info("ptimer: skipping remaining tests");
> +	}
> +}
>  
>  static u64 read_vtimer_counter(void)
>  {
> @@ -159,7 +181,6 @@ static void test_timer(struct timer_info *info)
>  	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
>  	u64 later = now + time_10s;
>  
> -
>  	/* Enable the timer, but schedule it for much later*/
>  	info->write_cval(later);
>  	isb();
> @@ -182,6 +203,9 @@ static void test_vtimer(void)
>  
>  static void test_ptimer(void)
>  {
> +	if (ptimer_unsupported)
> +		return;
> +
>  	report_prefix_push("ptimer-busy-loop");
>  	test_timer(&ptimer_info);
>  	report_prefix_pop();
> @@ -226,52 +250,27 @@ static void test_init(void)
>  	local_irq_enable();
>  }
>  
> -static void print_vtimer_info(void)
> +static void print_timer_info(void)
>  {
>  	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
> +
> +	if (!ptimer_unsupported) {
> +		printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
> +		printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
> +		printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
> +	}
> +
>  	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
>  	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
>  	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
>  }
>  
> -static void print_ptimer_info(void)
> -{
> -	printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
> -	printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
> -	printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
> -}
> -
> -
>  int main(int argc, char **argv)
>  {
> -	bool run_ptimer_test = false;
> -	bool run_vtimer_test = false;
> -
> -	/* Check if we should also check the physical timer */
> -	if (argc > 1) {
> -		if (strcmp(argv[1], "vtimer") == 0) {
> -			run_vtimer_test = true;
> -		} else if (strcmp(argv[1], "ptimer") == 0) {
> -			run_ptimer_test = true;
> -		} else {
> -			report_abort("Unknown option '%s'", argv[1]);
> -		}
> -	} else {
> -		run_vtimer_test = true;
> -	}
> -
> -	if (run_vtimer_test)
> -		print_vtimer_info();
> -	else if (run_ptimer_test)
> -		print_ptimer_info();
> -
> +	set_ptimer_unsupported();
> +	print_timer_info();
>  	test_init();
> -
> -	if (run_vtimer_test)
> -		test_vtimer();
> -	else if (run_ptimer_test)
> -		test_ptimer();
> -
> -
> +	test_ptimer();
> +	test_vtimer();
>  	return report_summary();
>  }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index d55e52e1a4c4f..bdfedf86b01cb 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -111,14 +111,7 @@ smp = $MAX_SMP
>  groups = psci
>  
>  # Timer tests
> -[vtimer]
> +[timer]
>  file = timer.flat
> -extra_params = -append 'vtimer'
> -groups = timer
> -timeout = 2s
> -
> -[ptimer]
> -file = timer.flat
> -extra_params = -append 'ptimer'
>  groups = timer
>  timeout = 2s

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Andrew Jones]

On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
> On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
> > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> > > Rearrange the code to be able to reuse as much as posible and add
> > > support for testing the physical timer as well.
> > > 
> > > Also change the default unittests configuration to run a separate vtimer
> > > and ptimer test so that older kernels without ptimer support just show a
> > > failure.
> > 
> > We could run tests for both the ptimer and vtimer in a single execution,
> > rather than splitting them and requiring the input, because the read of
> > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
> > applying the errata framework we can ensure that if we expect the read
> > to work, i.e. the host kernel is recent enough, then, if we still get
> > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
> > add on patch that makes the conversion. Let me know what you think.
> 
> The problem with this patch, is that we then report SKIP instead of
> FAIL, when we regress the kernel and actually break physical counter
> access (which I did while developing my series).
>

Well, as long as the cntp_ctl_el0 read isn't regressed into generating
an unknown exception, then the ptimer tests will always be run, reporting
failures as they should.

> I think something like this should be discovered by way of capabilities
> or hardcoding a kernel version.

That's possible already by making one more change (which I should have
made in the first place)

diff --git a/errata.txt b/errata.txt
index 5608a308ce7c9..8859d4f1d3860 100644
--- a/errata.txt
+++ b/errata.txt
@@ -4,4 +4,5 @@
 #---------------:-----------------------:--------------------------------------
 9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
 6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
+7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
 #---------------:-----------------------:--------------------------------------

With that change, when the test runtime system detects it's running on
a host with at least a 4.11 kernel, then it will automatically set
ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
errata set will even ensure the cntp_ctl_el0 read is tested.

> 
> Does kvm-unit-tests generally care about not reporting FAIL with earlier
> kernel versions?

Yes and no. kvm-unit-tests targets upstream KVM, thus if somebody gets
FAILs, then they should update KVM and try again before reporting them.
However, when it's not too difficult to detect a lacking feature, meaning
the tests should be skipped, then outputting SKIP is preferred, as it
allows upstream kvm-unit-tests to work nicely on downstream (older) KVM
without modification as well. Indeed, the errata framework was built with
that in mind. Ideally, upstream kvm-unit-tests can just work on downstream
KVM by only replacing the errata.txt file with one that properly
represents the downstream KVM under test.

Thanks,
drew

> > 
> > diff --git a/arm/timer.c b/arm/timer.c
> > index 33dfc6facc190..d0ba1e9a3bafa 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -7,6 +7,7 @@
> >   */
> >  #include <libcflat.h>
> >  #include <devicetree.h>
> > +#include <errata.h>
> >  #include <asm/processor.h>
> >  #include <asm/gic.h>
> >  #include <asm/io.h>
> > @@ -16,6 +17,27 @@
> >  #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
> >  
> >  static void *gic_ispendr;
> > +static bool ptimer_unsupported;
> > +
> > +static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
> > +{
> > +	ptimer_unsupported = true;
> > +	regs->pc += 4;
> > +}
> > +
> > +static void set_ptimer_unsupported(void)
> > +{
> > +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
> > +	read_sysreg(cntp_ctl_el0);
> > +	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, NULL);
> > +
> > +	if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
> > +		report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
> > +	} else if (ptimer_unsupported) {
> > +		report("ptimer: read CNTP_CTL_EL0", false);
> > +		report_info("ptimer: skipping remaining tests");
> > +	}
> > +}
> >  
> >  static u64 read_vtimer_counter(void)
> >  {
> > @@ -159,7 +181,6 @@ static void test_timer(struct timer_info *info)
> >  	u64 time_10s = read_sysreg(cntfrq_el0) * 10;
> >  	u64 later = now + time_10s;
> >  
> > -
> >  	/* Enable the timer, but schedule it for much later*/
> >  	info->write_cval(later);
> >  	isb();
> > @@ -182,6 +203,9 @@ static void test_vtimer(void)
> >  
> >  static void test_ptimer(void)
> >  {
> > +	if (ptimer_unsupported)
> > +		return;
> > +
> >  	report_prefix_push("ptimer-busy-loop");
> >  	test_timer(&ptimer_info);
> >  	report_prefix_pop();
> > @@ -226,52 +250,27 @@ static void test_init(void)
> >  	local_irq_enable();
> >  }
> >  
> > -static void print_vtimer_info(void)
> > +static void print_timer_info(void)
> >  {
> >  	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
> > +
> > +	if (!ptimer_unsupported) {
> > +		printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
> > +		printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
> > +		printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
> > +	}
> > +
> >  	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
> >  	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
> >  	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
> >  }
> >  
> > -static void print_ptimer_info(void)
> > -{
> > -	printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
> > -	printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
> > -	printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
> > -}
> > -
> > -
> >  int main(int argc, char **argv)
> >  {
> > -	bool run_ptimer_test = false;
> > -	bool run_vtimer_test = false;
> > -
> > -	/* Check if we should also check the physical timer */
> > -	if (argc > 1) {
> > -		if (strcmp(argv[1], "vtimer") == 0) {
> > -			run_vtimer_test = true;
> > -		} else if (strcmp(argv[1], "ptimer") == 0) {
> > -			run_ptimer_test = true;
> > -		} else {
> > -			report_abort("Unknown option '%s'", argv[1]);
> > -		}
> > -	} else {
> > -		run_vtimer_test = true;
> > -	}
> > -
> > -	if (run_vtimer_test)
> > -		print_vtimer_info();
> > -	else if (run_ptimer_test)
> > -		print_ptimer_info();
> > -
> > +	set_ptimer_unsupported();
> > +	print_timer_info();
> >  	test_init();
> > -
> > -	if (run_vtimer_test)
> > -		test_vtimer();
> > -	else if (run_ptimer_test)
> > -		test_ptimer();
> > -
> > -
> > +	test_ptimer();
> > +	test_vtimer();
> >  	return report_summary();
> >  }
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index d55e52e1a4c4f..bdfedf86b01cb 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -111,14 +111,7 @@ smp = $MAX_SMP
> >  groups = psci
> >  
> >  # Timer tests
> > -[vtimer]
> > +[timer]
> >  file = timer.flat
> > -extra_params = -append 'vtimer'
> > -groups = timer
> > -timeout = 2s
> > -
> > -[ptimer]
> > -file = timer.flat
> > -extra_params = -append 'ptimer'
> >  groups = timer
> >  timeout = 2s

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Christoffer Dall]

On Tue, Jul 18, 2017 at 03:23:24PM +0200, Andrew Jones wrote:
> On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
> > On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
> > > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> > > > Rearrange the code to be able to reuse as much as posible and add
> > > > support for testing the physical timer as well.
> > > > 
> > > > Also change the default unittests configuration to run a separate vtimer
> > > > and ptimer test so that older kernels without ptimer support just show a
> > > > failure.
> > > 
> > > We could run tests for both the ptimer and vtimer in a single execution,
> > > rather than splitting them and requiring the input, because the read of
> > > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
> > > applying the errata framework we can ensure that if we expect the read
> > > to work, i.e. the host kernel is recent enough, then, if we still get
> > > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
> > > add on patch that makes the conversion. Let me know what you think.
> > 
> > The problem with this patch, is that we then report SKIP instead of
> > FAIL, when we regress the kernel and actually break physical counter
> > access (which I did while developing my series).
> >
> 
> Well, as long as the cntp_ctl_el0 read isn't regressed into generating
> an unknown exception, then the ptimer tests will always be run, reporting
> failures as they should.
> 

And that is exactly what we've done a couple of times around, because
VHE changes the layout of the trap control register to EL2, and the way
we handle traps to KVM of the physical counter register is to inject an
undefined exception...

> > I think something like this should be discovered by way of capabilities
> > or hardcoding a kernel version.
> 
> That's possible already by making one more change (which I should have
> made in the first place)
> 
> diff --git a/errata.txt b/errata.txt
> index 5608a308ce7c9..8859d4f1d3860 100644
> --- a/errata.txt
> +++ b/errata.txt
> @@ -4,4 +4,5 @@
>  #---------------:-----------------------:--------------------------------------
>  9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
>  6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
> +7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
>  #---------------:-----------------------:--------------------------------------
> 
> With that change, when the test runtime system detects it's running on
> a host with at least a 4.11 kernel, then it will automatically set
> ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
> errata set will even ensure the cntp_ctl_el0 read is tested.
> 

ah, ok then that makes perfect sense.

I'm a little confused about the logic though, if we regress the physical
counter access on a newer kernel in a way that gives you an undefined
exception, will we get FAIL or SKIP?

We should get FAIL.

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Andrew Jones]

On Tue, Jul 18, 2017 at 03:31:03PM +0200, Christoffer Dall wrote:
> On Tue, Jul 18, 2017 at 03:23:24PM +0200, Andrew Jones wrote:
> > On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
> > > On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
> > > > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> > > > > Rearrange the code to be able to reuse as much as posible and add
> > > > > support for testing the physical timer as well.
> > > > > 
> > > > > Also change the default unittests configuration to run a separate vtimer
> > > > > and ptimer test so that older kernels without ptimer support just show a
> > > > > failure.
> > > > 
> > > > We could run tests for both the ptimer and vtimer in a single execution,
> > > > rather than splitting them and requiring the input, because the read of
> > > > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
> > > > applying the errata framework we can ensure that if we expect the read
> > > > to work, i.e. the host kernel is recent enough, then, if we still get
> > > > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
> > > > add on patch that makes the conversion. Let me know what you think.
> > > 
> > > The problem with this patch, is that we then report SKIP instead of
> > > FAIL, when we regress the kernel and actually break physical counter
> > > access (which I did while developing my series).
> > >
> > 
> > Well, as long as the cntp_ctl_el0 read isn't regressed into generating
> > an unknown exception, then the ptimer tests will always be run, reporting
> > failures as they should.
> > 
> 
> And that is exactly what we've done a couple of times around, because
> VHE changes the layout of the trap control register to EL2, and the way
> we handle traps to KVM of the physical counter register is to inject an
> undefined exception...
> 
> > > I think something like this should be discovered by way of capabilities
> > > or hardcoding a kernel version.
> > 
> > That's possible already by making one more change (which I should have
> > made in the first place)
> > 
> > diff --git a/errata.txt b/errata.txt
> > index 5608a308ce7c9..8859d4f1d3860 100644
> > --- a/errata.txt
> > +++ b/errata.txt
> > @@ -4,4 +4,5 @@
> >  #---------------:-----------------------:--------------------------------------
> >  9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
> >  6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
> > +7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
> >  #---------------:-----------------------:--------------------------------------
> > 
> > With that change, when the test runtime system detects it's running on
> > a host with at least a 4.11 kernel, then it will automatically set
> > ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
> > errata set will even ensure the cntp_ctl_el0 read is tested.
> > 
> 
> ah, ok then that makes perfect sense.
> 
> I'm a little confused about the logic though, if we regress the physical
> counter access on a newer kernel in a way that gives you an undefined
> exception, will we get FAIL or SKIP?
> 
> We should get FAIL.

We'll get FAIL as long as the user doesn't override ERRATA_7b6b46311a85
to be 'n'. See the if-else in set_ptimer_unsupported()

  if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
          report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
  } else if (ptimer_unsupported) {
          report("ptimer: read CNTP_CTL_EL0", false);
          report_info("ptimer: skipping remaining tests");
  }

I was guessing we'd want to skip the remaining tests if we can't
even read the register, but it could be reworked to try every
ptimer test when ERRATA_7b6b46311a85=y as well.

Thanks,
drew

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Christoffer Dall]

On Tue, Jul 18, 2017 at 3:50 PM, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Jul 18, 2017 at 03:31:03PM +0200, Christoffer Dall wrote:
>> On Tue, Jul 18, 2017 at 03:23:24PM +0200, Andrew Jones wrote:
>> > On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
>> > > On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
>> > > > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
>> > > > > Rearrange the code to be able to reuse as much as posible and add
>> > > > > support for testing the physical timer as well.
>> > > > >
>> > > > > Also change the default unittests configuration to run a separate vtimer
>> > > > > and ptimer test so that older kernels without ptimer support just show a
>> > > > > failure.
>> > > >
>> > > > We could run tests for both the ptimer and vtimer in a single execution,
>> > > > rather than splitting them and requiring the input, because the read of
>> > > > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
>> > > > applying the errata framework we can ensure that if we expect the read
>> > > > to work, i.e. the host kernel is recent enough, then, if we still get
>> > > > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
>> > > > add on patch that makes the conversion. Let me know what you think.
>> > >
>> > > The problem with this patch, is that we then report SKIP instead of
>> > > FAIL, when we regress the kernel and actually break physical counter
>> > > access (which I did while developing my series).
>> > >
>> >
>> > Well, as long as the cntp_ctl_el0 read isn't regressed into generating
>> > an unknown exception, then the ptimer tests will always be run, reporting
>> > failures as they should.
>> >
>>
>> And that is exactly what we've done a couple of times around, because
>> VHE changes the layout of the trap control register to EL2, and the way
>> we handle traps to KVM of the physical counter register is to inject an
>> undefined exception...
>>
>> > > I think something like this should be discovered by way of capabilities
>> > > or hardcoding a kernel version.
>> >
>> > That's possible already by making one more change (which I should have
>> > made in the first place)
>> >
>> > diff --git a/errata.txt b/errata.txt
>> > index 5608a308ce7c9..8859d4f1d3860 100644
>> > --- a/errata.txt
>> > +++ b/errata.txt
>> > @@ -4,4 +4,5 @@
>> >  #---------------:-----------------------:--------------------------------------
>> >  9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
>> >  6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
>> > +7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
>> >  #---------------:-----------------------:--------------------------------------
>> >
>> > With that change, when the test runtime system detects it's running on
>> > a host with at least a 4.11 kernel, then it will automatically set
>> > ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
>> > errata set will even ensure the cntp_ctl_el0 read is tested.
>> >
>>
>> ah, ok then that makes perfect sense.
>>
>> I'm a little confused about the logic though, if we regress the physical
>> counter access on a newer kernel in a way that gives you an undefined
>> exception, will we get FAIL or SKIP?
>>
>> We should get FAIL.
>
> We'll get FAIL as long as the user doesn't override ERRATA_7b6b46311a85
> to be 'n'. See the if-else in set_ptimer_unsupported()
>
>   if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
>           report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
>   } else if (ptimer_unsupported) {
>           report("ptimer: read CNTP_CTL_EL0", false);
>           report_info("ptimer: skipping remaining tests");
>   }

so report("...", false); means FAIL?

If so, ok :)

>
> I was guessing we'd want to skip the remaining tests if we can't
> even read the register, but it could be reworked to try every
> ptimer test when ERRATA_7b6b46311a85=y as well.
>

It makes sense to skip the rest, agreed.

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Andrew Jones]

On Tue, Jul 18, 2017 at 04:15:23PM +0200, Christoffer Dall wrote:
> On Tue, Jul 18, 2017 at 3:50 PM, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Jul 18, 2017 at 03:31:03PM +0200, Christoffer Dall wrote:
> >> On Tue, Jul 18, 2017 at 03:23:24PM +0200, Andrew Jones wrote:
> >> > On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
> >> > > On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
> >> > > > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
> >> > > > > Rearrange the code to be able to reuse as much as posible and add
> >> > > > > support for testing the physical timer as well.
> >> > > > >
> >> > > > > Also change the default unittests configuration to run a separate vtimer
> >> > > > > and ptimer test so that older kernels without ptimer support just show a
> >> > > > > failure.
> >> > > >
> >> > > > We could run tests for both the ptimer and vtimer in a single execution,
> >> > > > rather than splitting them and requiring the input, because the read of
> >> > > > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
> >> > > > applying the errata framework we can ensure that if we expect the read
> >> > > > to work, i.e. the host kernel is recent enough, then, if we still get
> >> > > > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
> >> > > > add on patch that makes the conversion. Let me know what you think.
> >> > >
> >> > > The problem with this patch, is that we then report SKIP instead of
> >> > > FAIL, when we regress the kernel and actually break physical counter
> >> > > access (which I did while developing my series).
> >> > >
> >> >
> >> > Well, as long as the cntp_ctl_el0 read isn't regressed into generating
> >> > an unknown exception, then the ptimer tests will always be run, reporting
> >> > failures as they should.
> >> >
> >>
> >> And that is exactly what we've done a couple of times around, because
> >> VHE changes the layout of the trap control register to EL2, and the way
> >> we handle traps to KVM of the physical counter register is to inject an
> >> undefined exception...
> >>
> >> > > I think something like this should be discovered by way of capabilities
> >> > > or hardcoding a kernel version.
> >> >
> >> > That's possible already by making one more change (which I should have
> >> > made in the first place)
> >> >
> >> > diff --git a/errata.txt b/errata.txt
> >> > index 5608a308ce7c9..8859d4f1d3860 100644
> >> > --- a/errata.txt
> >> > +++ b/errata.txt
> >> > @@ -4,4 +4,5 @@
> >> >  #---------------:-----------------------:--------------------------------------
> >> >  9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
> >> >  6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
> >> > +7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
> >> >  #---------------:-----------------------:--------------------------------------
> >> >
> >> > With that change, when the test runtime system detects it's running on
> >> > a host with at least a 4.11 kernel, then it will automatically set
> >> > ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
> >> > errata set will even ensure the cntp_ctl_el0 read is tested.
> >> >
> >>
> >> ah, ok then that makes perfect sense.
> >>
> >> I'm a little confused about the logic though, if we regress the physical
> >> counter access on a newer kernel in a way that gives you an undefined
> >> exception, will we get FAIL or SKIP?
> >>
> >> We should get FAIL.
> >
> > We'll get FAIL as long as the user doesn't override ERRATA_7b6b46311a85
> > to be 'n'. See the if-else in set_ptimer_unsupported()
> >
> >   if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
> >           report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
> >   } else if (ptimer_unsupported) {
> >           report("ptimer: read CNTP_CTL_EL0", false);
> >           report_info("ptimer: skipping remaining tests");
> >   }
> 
> so report("...", false); means FAIL?

Yup, first argument following fmt to report() is the test result. We
should probably create report_pass/fail() wrappers.

Thanks,
drew

Re: [PATCH kvm-unit-tests 3/3] arm64: timer: Add support for phys timer testing

[Christoffer Dall]

On Tue, Jul 18, 2017 at 4:29 PM, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Jul 18, 2017 at 04:15:23PM +0200, Christoffer Dall wrote:
>> On Tue, Jul 18, 2017 at 3:50 PM, Andrew Jones <drjones@redhat.com> wrote:
>> > On Tue, Jul 18, 2017 at 03:31:03PM +0200, Christoffer Dall wrote:
>> >> On Tue, Jul 18, 2017 at 03:23:24PM +0200, Andrew Jones wrote:
>> >> > On Tue, Jul 18, 2017 at 03:01:53PM +0200, Christoffer Dall wrote:
>> >> > > On Tue, Jul 18, 2017 at 02:09:57PM +0200, Andrew Jones wrote:
>> >> > > > On Thu, Jul 13, 2017 at 09:20:09PM +0200, Christoffer Dall wrote:
>> >> > > > > Rearrange the code to be able to reuse as much as posible and add
>> >> > > > > support for testing the physical timer as well.
>> >> > > > >
>> >> > > > > Also change the default unittests configuration to run a separate vtimer
>> >> > > > > and ptimer test so that older kernels without ptimer support just show a
>> >> > > > > failure.
>> >> > > >
>> >> > > > We could run tests for both the ptimer and vtimer in a single execution,
>> >> > > > rather than splitting them and requiring the input, because the read of
>> >> > > > cntp_ctl_el0 will predictably cause an UNKNOWN exception. Also, by
>> >> > > > applying the errata framework we can ensure that if we expect the read
>> >> > > > to work, i.e. the host kernel is recent enough, then, if we still get
>> >> > > > an UNKNOWN exception, we can report FAIL instead of SKIP. Below is an
>> >> > > > add on patch that makes the conversion. Let me know what you think.
>> >> > >
>> >> > > The problem with this patch, is that we then report SKIP instead of
>> >> > > FAIL, when we regress the kernel and actually break physical counter
>> >> > > access (which I did while developing my series).
>> >> > >
>> >> >
>> >> > Well, as long as the cntp_ctl_el0 read isn't regressed into generating
>> >> > an unknown exception, then the ptimer tests will always be run, reporting
>> >> > failures as they should.
>> >> >
>> >>
>> >> And that is exactly what we've done a couple of times around, because
>> >> VHE changes the layout of the trap control register to EL2, and the way
>> >> we handle traps to KVM of the physical counter register is to inject an
>> >> undefined exception...
>> >>
>> >> > > I think something like this should be discovered by way of capabilities
>> >> > > or hardcoding a kernel version.
>> >> >
>> >> > That's possible already by making one more change (which I should have
>> >> > made in the first place)
>> >> >
>> >> > diff --git a/errata.txt b/errata.txt
>> >> > index 5608a308ce7c9..8859d4f1d3860 100644
>> >> > --- a/errata.txt
>> >> > +++ b/errata.txt
>> >> > @@ -4,4 +4,5 @@
>> >> >  #---------------:-----------------------:--------------------------------------
>> >> >  9e3f7a296940   : 4.9                   : arm64: KVM: pmu: Fix AArch32 cycle counter access
>> >> >  6c7a5dce22b3   : 4.12                  : KVM: arm/arm64: fix races in kvm_psci_vcpu_on
>> >> > +7b6b46311a85   : 4.11                  : KVM: arm/arm64: Emulate the EL1 phys timer registers
>> >> >  #---------------:-----------------------:--------------------------------------
>> >> >
>> >> > With that change, when the test runtime system detects it's running on
>> >> > a host with at least a 4.11 kernel, then it will automatically set
>> >> > ERRATA_7b6b46311a85=y (unless overridden by the user). Having that
>> >> > errata set will even ensure the cntp_ctl_el0 read is tested.
>> >> >
>> >>
>> >> ah, ok then that makes perfect sense.
>> >>
>> >> I'm a little confused about the logic though, if we regress the physical
>> >> counter access on a newer kernel in a way that gives you an undefined
>> >> exception, will we get FAIL or SKIP?
>> >>
>> >> We should get FAIL.
>> >
>> > We'll get FAIL as long as the user doesn't override ERRATA_7b6b46311a85
>> > to be 'n'. See the if-else in set_ptimer_unsupported()
>> >
>> >   if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
>> >           report_skip("Skipping ptimer tests. Set ERRATA_7b6b46311a85=y to enable.");
>> >   } else if (ptimer_unsupported) {
>> >           report("ptimer: read CNTP_CTL_EL0", false);
>> >           report_info("ptimer: skipping remaining tests");
>> >   }
>>
>> so report("...", false); means FAIL?
>
> Yup, first argument following fmt to report() is the test result. We
> should probably create report_pass/fail() wrappers.
>
Yes, for incompetent reviewers like me :)

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Paolo Bonzini]

On 18/07/2017 14:15, Andrew Jones wrote:
>>
>> If we want to add a "the platform provides a timer with 56 valid bits in
>> the counter and compare register", then I think it should be a separate
>> test, and the the user can see that "basic stuff works", "architecture
>> compliance not so much" and shrug accordingly.
> Two separate tests sounds good to me. Although, if the value of 'now' is
> large enough that now + 10s will set bit 31, then a mustang run (at least
> mine) will fail - and most likely cause a lot of confusion, since it
> normally does not. We should probably attempt to address that known issue
> in some way as well.

Is the value relative to vm startup or host startup?

Paolo

Re: [PATCH kvm-unit-tests 0/3] Add physical timer test

[Paolo Bonzini]

On 13/07/2017 21:20, Christoffer Dall wrote:
> Add a test for the vtimer. I've tested on
>  accel=tcg
>  accel=kvm                     : on seattle, and mustang
>  accel=kvm,kernel-irqchip=off  : on mustang
> 
> I first fix two issues I had running the basic timer test on APM mustang
> on using TCG.  I wonder why the vtimer tests worked using TCG for Drew,
> since they didn't work for me, and I don't see how they would have
> without patch 1.
> 
> Then I introduce a test for the physical timer.  If you run the ptimer
> test on a kernel before support for the physical timers was added you
> get a nice error message plus some spamming in your kernel log.
> 
> 
> Christoffer Dall (3):
>   arm64: timer: Fix vtimer interrupt test
>   arm64: timer: Fix test on APM X-Gene
>   arm64: timer: Add support for phys timer testing
> 
>  arm/timer.c       | 205 +++++++++++++++++++++++++++++++++++++++++++++---------
>  arm/unittests.cfg |   9 ++-
>  2 files changed, 182 insertions(+), 32 deletions(-)
> 


Applied, thanks!

Paolo

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Christoffer Dall]

On Mon, Jul 24, 2017 at 7:13 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 18/07/2017 14:15, Andrew Jones wrote:
>>>
>>> If we want to add a "the platform provides a timer with 56 valid bits in
>>> the counter and compare register", then I think it should be a separate
>>> test, and the the user can see that "basic stuff works", "architecture
>>> compliance not so much" and shrug accordingly.
>> Two separate tests sounds good to me. Although, if the value of 'now' is
>> large enough that now + 10s will set bit 31, then a mustang run (at least
>> mine) will fail - and most likely cause a lot of confusion, since it
>> normally does not. We should probably attempt to address that known issue
>> in some way as well.
>
> Is the value relative to vm startup or host startup?
>
For the virtual timer, KVM currently resets the counter to zero (so VM
startup) and for the physical timer it's since host startup.

I confirmed the behavior Drew reported on my mustang too, but it only
appears to apply for the vtimer when writing cval with bit 31 set, not
for the ptimer.  I was thinking that this may be a problem with KVM,
some sort of signed bit extension problem, but since the problem
doesn't show up on other platforms, it may just be a problem there.
Shrug.

Anyway, since it works with the ptimer, and we should have at least 56
bits of counter space at ~50 MHz, we should be fine here.

Thanks,
-Christoffer

Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

[Christoffer Dall]

On Mon, Jul 24, 2017 at 11:25:50PM +0200, Christoffer Dall wrote:
> On Mon, Jul 24, 2017 at 7:13 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 18/07/2017 14:15, Andrew Jones wrote:
> >>>
> >>> If we want to add a "the platform provides a timer with 56 valid bits in
> >>> the counter and compare register", then I think it should be a separate
> >>> test, and the the user can see that "basic stuff works", "architecture
> >>> compliance not so much" and shrug accordingly.
> >> Two separate tests sounds good to me. Although, if the value of 'now' is
> >> large enough that now + 10s will set bit 31, then a mustang run (at least
> >> mine) will fail - and most likely cause a lot of confusion, since it
> >> normally does not. We should probably attempt to address that known issue
> >> in some way as well.
> >
> > Is the value relative to vm startup or host startup?
> >
> For the virtual timer, KVM currently resets the counter to zero (so VM
> startup) and for the physical timer it's since host startup.
> 
> I confirmed the behavior Drew reported on my mustang too, but it only
> appears to apply for the vtimer when writing cval with bit 31 set, not
> for the ptimer.  I was thinking that this may be a problem with KVM,
> some sort of signed bit extension problem, but since the problem
> doesn't show up on other platforms, it may just be a problem there.

ok, so I did some digging here.  The problem doesn't seem to be with bit
31 specifically, but rather that the Mustang cannot handle a larger
difference between cval and cnt, than (1 << 31) - 1.

I verified this by writing to tval instead and increasing its value more
and more.

Furthermore, I also added a delay loop, that waits until the counter
reaches 0xffffffff and then program cval with something later, which
works just fine.

I also looked at the hardware registers and observe things like:

cval: 0x100000000 cnt_ctl: 0x5 cnt: 0x225a6e

Which means that the timer sets ISTATUS even though it clearly shouldn't
fire.

The explanation why Linux always (seems to) work on the platform is
likely that Linux never programs a timer further out than ~80 seconds.

Broken hardware, oh well.

Thanks,
-Christoffer