Re: [PATCH] arm: Add simple arch timer test

[Alexander Graf]

On 16/11/2016 16:54, Andrew Jones wrote:

On Mon, Sep 19, 2016 at 04:52:01PM +0200, Andrew Jones wrote:

On Mon, Sep 19, 2016 at 01:44:40PM +0200, Alexander Graf wrote:

All virtualization capable ARM cores support the ARM architected virtual timer.

This patch adds minimalistic checks whether we can fire a virtual timer event
using them. It does not actually trigger an interrupt yet, as infrastructure
for that is missing. However, it does check whether the timer pin is marked as
pending on the GIC.

Signed-off-by: Alexander Graf 
---
 arm/Makefile.arm64 |   2 +-
 arm/vtimer-test.c  | 122 +
 2 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 arm/vtimer-test.c

diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
index 0b0761c..5b3fbe8 100644
--- a/arm/Makefile.arm64
+++ b/arm/Makefile.arm64
@@ -12,7 +12,7 @@ cflatobjs += lib/arm64/processor.o
 cflatobjs += lib/arm64/spinlock.o

 # arm64 specific tests
-tests =
+tests = $(TEST_DIR)/vtimer-test.flat

 include $(TEST_DIR)/Makefile.common

diff --git a/arm/vtimer-test.c b/arm/vtimer-test.c
new file mode 100644
index 000..a3e24ce
--- /dev/null
+++ b/arm/vtimer-test.c
@@ -0,0 +1,122 @@
+/*
+ * Unit tests for the virtual timer on the ARM virt machine
+ *
+ * Copyright (C) 2016, Alexander Graf 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include 
+#include 
+
+#define VTIMER_CTL_ENABLE  (1 << 0)
+#define VTIMER_CTL_IMASK   (1 << 1)
+#define VTIMER_CTL_ISTATUS (1 << 2)
+
+#define VIRT_GIC_DIST_BASE 0x0800
+#define GIC_DIST_PENDING_SET0x200
+#define GIC_DIST_PENDING_CLEAR  0x280
+#define GIC_DIST_ACTIVE_SET 0x300
+#define GIC_DIST_ACTIVE_CLEAR   0x380
+
+#define VIRT_GIC_CPU_BASE  0x0801


Once I get my arm/gic series in we won't need to hard code
the gic base addresses and these gic register offsets will
also be provided.


+
+#define ARCH_TIMER_VIRT_IRQ   11
+#define ARCH_TIMER_S_EL1_IRQ  13
+#define ARCH_TIMER_NS_EL1_IRQ 14
+#define ARCH_TIMER_NS_EL2_IRQ 10


We can pull these out of the DT.


+
+static void write_vtimer_ctl(u64 val)
+{
+   asm volatile("msr cntv_ctl_el0, %0" : : "r" (val));
+}
+
+static void write_vtimer_cval(u64 val)
+{
+   asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
+}
+
+static u64 read_vtimer_ctl(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntv_ctl_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_cval(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntv_cval_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_val(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntvct_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_freq(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
+   return val;
+}
+
+static bool gic_vtimer_pending(void)
+{
+   u32 *pending = (u32*)(VIRT_GIC_DIST_BASE + GIC_DIST_PENDING_SET);
+   return (readl(pending) & (1 << (ARCH_TIMER_VIRT_IRQ + 16)));


nit: don't need the outer ()

I think I'd like to add the PPI(irq) macro we added to QEMU for
the +16's.


+}
+
+static bool test_cval_10msec(void)
+{
+   u64 time_10ms = read_vtimer_freq() / 100;
+   u64 time_1us = time_10ms / 1;
+   u64 before_timer = read_vtimer_val();
+   u64 after_timer, latency;
+
+   /* Program timer to fire in 10ms */
+   write_vtimer_cval(before_timer + time_10ms);
+
+   /* Wait for the timer to fire */
+   while (!(read_vtimer_ctl() & VTIMER_CTL_ISTATUS)) ;


If emulation fails to set the status bit then we'll spin forever.
That's OK if we set the test timeout (arm/unittests.cfg:timeout
for this test appropriately) Or, maybe can add a trial count here
that's sufficiently large?


+
+   /* It fired, check how long it took */
+   after_timer = read_vtimer_val();
+
+   latency = (after_timer - (before_timer + time_10ms));


nit: don't need the outer ()

Shouldn't latency be signed so we can see if the status bit was
set too soon? Or just compare time_10ms with after - before?


+   printf("After timer: 0x%lx\n", after_timer);
+   printf("Expected   : 0x%lx\n", before_timer + time_10ms);
+   printf("Latency: %ld us\n", latency / time_1us);
+
+   return latency < time_10ms;


Does this mean that the threshold for success is 10ms? If so,
then that's not too large?


+}
+
+static void test_vtimer(void)
+{
+   printf("\n=== vtimer with busy loop ===\n");


I guess this is a subtest header, i.e. this file will get
other subtests and headers like this one will divide up
the test output. That's fine, but new, at least for arm tests.
Currently we're dividing subtests with report prefixes. In
this case, instead of the above printf, we'd do

 report_prefix_push("busy-loop");

or whatever the prefix should be. At the end of the function
we'd pop the prefix. I 

Re: [PATCH] arm: Add simple arch timer test

[Andrew Jones]

On Mon, Sep 19, 2016 at 04:52:01PM +0200, Andrew Jones wrote:
> On Mon, Sep 19, 2016 at 01:44:40PM +0200, Alexander Graf wrote:
> > All virtualization capable ARM cores support the ARM architected virtual 
> > timer.
> > 
> > This patch adds minimalistic checks whether we can fire a virtual timer 
> > event
> > using them. It does not actually trigger an interrupt yet, as infrastructure
> > for that is missing. However, it does check whether the timer pin is marked 
> > as
> > pending on the GIC.
> > 
> > Signed-off-by: Alexander Graf 
> > ---
> >  arm/Makefile.arm64 |   2 +-
> >  arm/vtimer-test.c  | 122 
> > +
> >  2 files changed, 123 insertions(+), 1 deletion(-)
> >  create mode 100644 arm/vtimer-test.c
> > 
> > diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> > index 0b0761c..5b3fbe8 100644
> > --- a/arm/Makefile.arm64
> > +++ b/arm/Makefile.arm64
> > @@ -12,7 +12,7 @@ cflatobjs += lib/arm64/processor.o
> >  cflatobjs += lib/arm64/spinlock.o
> >  
> >  # arm64 specific tests
> > -tests =
> > +tests = $(TEST_DIR)/vtimer-test.flat
> >  
> >  include $(TEST_DIR)/Makefile.common
> >  
> > diff --git a/arm/vtimer-test.c b/arm/vtimer-test.c
> > new file mode 100644
> > index 000..a3e24ce
> > --- /dev/null
> > +++ b/arm/vtimer-test.c
> > @@ -0,0 +1,122 @@
> > +/*
> > + * Unit tests for the virtual timer on the ARM virt machine
> > + *
> > + * Copyright (C) 2016, Alexander Graf 
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#include 
> > +#include 
> > +
> > +#define VTIMER_CTL_ENABLE  (1 << 0)
> > +#define VTIMER_CTL_IMASK   (1 << 1)
> > +#define VTIMER_CTL_ISTATUS (1 << 2)
> > +
> > +#define VIRT_GIC_DIST_BASE 0x0800
> > +#define GIC_DIST_PENDING_SET0x200
> > +#define GIC_DIST_PENDING_CLEAR  0x280
> > +#define GIC_DIST_ACTIVE_SET 0x300
> > +#define GIC_DIST_ACTIVE_CLEAR   0x380
> > +
> > +#define VIRT_GIC_CPU_BASE  0x0801
> 
> Once I get my arm/gic series in we won't need to hard code
> the gic base addresses and these gic register offsets will
> also be provided.
> 
> > +
> > +#define ARCH_TIMER_VIRT_IRQ   11
> > +#define ARCH_TIMER_S_EL1_IRQ  13
> > +#define ARCH_TIMER_NS_EL1_IRQ 14
> > +#define ARCH_TIMER_NS_EL2_IRQ 10
> 
> We can pull these out of the DT.
> 
> > +
> > +static void write_vtimer_ctl(u64 val)
> > +{
> > +   asm volatile("msr cntv_ctl_el0, %0" : : "r" (val));
> > +}
> > +
> > +static void write_vtimer_cval(u64 val)
> > +{
> > +   asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
> > +}
> > +
> > +static u64 read_vtimer_ctl(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntv_ctl_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_cval(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntv_cval_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_val(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntvct_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_freq(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static bool gic_vtimer_pending(void)
> > +{
> > +   u32 *pending = (u32*)(VIRT_GIC_DIST_BASE + GIC_DIST_PENDING_SET);
> > +   return (readl(pending) & (1 << (ARCH_TIMER_VIRT_IRQ + 16)));
> 
> nit: don't need the outer ()
> 
> I think I'd like to add the PPI(irq) macro we added to QEMU for
> the +16's.
> 
> > +}
> > +
> > +static bool test_cval_10msec(void)
> > +{
> > +   u64 time_10ms = read_vtimer_freq() / 100;
> > +   u64 time_1us = time_10ms / 1;
> > +   u64 before_timer = read_vtimer_val();
> > +   u64 after_timer, latency;
> > +
> > +   /* Program timer to fire in 10ms */
> > +   write_vtimer_cval(before_timer + time_10ms);
> > +
> > +   /* Wait for the timer to fire */
> > +   while (!(read_vtimer_ctl() & VTIMER_CTL_ISTATUS)) ;
> 
> If emulation fails to set the status bit then we'll spin forever.
> That's OK if we set the test timeout (arm/unittests.cfg:timeout
> for this test appropriately) Or, maybe can add a trial count here
> that's sufficiently large?
> 
> > +
> > +   /* It fired, check how long it took */
> > +   after_timer = read_vtimer_val();
> > +
> > +   latency = (after_timer - (before_timer + time_10ms));
> 
> nit: don't need the outer ()
> 
> Shouldn't latency be signed so we can see if the status bit was
> set too soon? Or just compare time_10ms with after - before?
> 
> > +   printf("After timer: 0x%lx\n", after_timer);
> > +   printf("Expected   : 0x%lx\n", before_timer + time_10ms);
> > +   printf("Latency: %ld us\n", latency / time_1us);
> > +
> > +   return latency < time_10ms;
> 
> Does this mean that the threshold for success is 10ms? If so,
> then that's not too large?
> 
> > +}
> > +
> > +static void test_vtimer(void)
> > +{
> > +   printf("\n=== vtimer with busy 

Re: [PATCH] arm: Add simple arch timer test

[Andrew Jones]

On Mon, Sep 19, 2016 at 01:44:40PM +0200, Alexander Graf wrote:
> All virtualization capable ARM cores support the ARM architected virtual 
> timer.
> 
> This patch adds minimalistic checks whether we can fire a virtual timer event
> using them. It does not actually trigger an interrupt yet, as infrastructure
> for that is missing. However, it does check whether the timer pin is marked as
> pending on the GIC.
> 
> Signed-off-by: Alexander Graf 
> ---
>  arm/Makefile.arm64 |   2 +-
>  arm/vtimer-test.c  | 122 
> +
>  2 files changed, 123 insertions(+), 1 deletion(-)
>  create mode 100644 arm/vtimer-test.c
> 
> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> index 0b0761c..5b3fbe8 100644
> --- a/arm/Makefile.arm64
> +++ b/arm/Makefile.arm64
> @@ -12,7 +12,7 @@ cflatobjs += lib/arm64/processor.o
>  cflatobjs += lib/arm64/spinlock.o
>  
>  # arm64 specific tests
> -tests =
> +tests = $(TEST_DIR)/vtimer-test.flat
>  
>  include $(TEST_DIR)/Makefile.common
>  
> diff --git a/arm/vtimer-test.c b/arm/vtimer-test.c
> new file mode 100644
> index 000..a3e24ce
> --- /dev/null
> +++ b/arm/vtimer-test.c
> @@ -0,0 +1,122 @@
> +/*
> + * Unit tests for the virtual timer on the ARM virt machine
> + *
> + * Copyright (C) 2016, Alexander Graf 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include 
> +#include 
> +
> +#define VTIMER_CTL_ENABLE  (1 << 0)
> +#define VTIMER_CTL_IMASK   (1 << 1)
> +#define VTIMER_CTL_ISTATUS (1 << 2)
> +
> +#define VIRT_GIC_DIST_BASE 0x0800
> +#define GIC_DIST_PENDING_SET0x200
> +#define GIC_DIST_PENDING_CLEAR  0x280
> +#define GIC_DIST_ACTIVE_SET 0x300
> +#define GIC_DIST_ACTIVE_CLEAR   0x380
> +
> +#define VIRT_GIC_CPU_BASE  0x0801

Once I get my arm/gic series in we won't need to hard code
the gic base addresses and these gic register offsets will
also be provided.

> +
> +#define ARCH_TIMER_VIRT_IRQ   11
> +#define ARCH_TIMER_S_EL1_IRQ  13
> +#define ARCH_TIMER_NS_EL1_IRQ 14
> +#define ARCH_TIMER_NS_EL2_IRQ 10

We can pull these out of the DT.

> +
> +static void write_vtimer_ctl(u64 val)
> +{
> + asm volatile("msr cntv_ctl_el0, %0" : : "r" (val));
> +}
> +
> +static void write_vtimer_cval(u64 val)
> +{
> + asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
> +}
> +
> +static u64 read_vtimer_ctl(void)
> +{
> + u64 val;
> + asm volatile("mrs %0, cntv_ctl_el0" : "=r" (val));
> + return val;
> +}
> +
> +static u64 read_vtimer_cval(void)
> +{
> + u64 val;
> + asm volatile("mrs %0, cntv_cval_el0" : "=r" (val));
> + return val;
> +}
> +
> +static u64 read_vtimer_val(void)
> +{
> + u64 val;
> + asm volatile("mrs %0, cntvct_el0" : "=r" (val));
> + return val;
> +}
> +
> +static u64 read_vtimer_freq(void)
> +{
> + u64 val;
> + asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
> + return val;
> +}
> +
> +static bool gic_vtimer_pending(void)
> +{
> + u32 *pending = (u32*)(VIRT_GIC_DIST_BASE + GIC_DIST_PENDING_SET);
> + return (readl(pending) & (1 << (ARCH_TIMER_VIRT_IRQ + 16)));

nit: don't need the outer ()

I think I'd like to add the PPI(irq) macro we added to QEMU for
the +16's.

> +}
> +
> +static bool test_cval_10msec(void)
> +{
> + u64 time_10ms = read_vtimer_freq() / 100;
> + u64 time_1us = time_10ms / 1;
> + u64 before_timer = read_vtimer_val();
> + u64 after_timer, latency;
> +
> + /* Program timer to fire in 10ms */
> + write_vtimer_cval(before_timer + time_10ms);
> +
> + /* Wait for the timer to fire */
> + while (!(read_vtimer_ctl() & VTIMER_CTL_ISTATUS)) ;

If emulation fails to set the status bit then we'll spin forever.
That's OK if we set the test timeout (arm/unittests.cfg:timeout
for this test appropriately) Or, maybe can add a trial count here
that's sufficiently large?

> +
> + /* It fired, check how long it took */
> + after_timer = read_vtimer_val();
> +
> + latency = (after_timer - (before_timer + time_10ms));

nit: don't need the outer ()

Shouldn't latency be signed so we can see if the status bit was
set too soon? Or just compare time_10ms with after - before?

> + printf("After timer: 0x%lx\n", after_timer);
> + printf("Expected   : 0x%lx\n", before_timer + time_10ms);
> + printf("Latency: %ld us\n", latency / time_1us);
> +
> + return latency < time_10ms;

Does this mean that the threshold for success is 10ms? If so,
then that's not too large?

> +}
> +
> +static void test_vtimer(void)
> +{
> + printf("\n=== vtimer with busy loop ===\n");

I guess this is a subtest header, i.e. this file will get
other subtests and headers like this one will divide up
the test output. That's fine, but new, at least for arm tests.
Currently we're dividing subtests with report prefixes. In
this case, instead of the above printf, we'd do

 report_prefix_push("busy-loop");

or