[PATCH v2 0/2] arch_timers patches to enable KVM support

[PATCH v2 0/2] arch_timers patches to enable KVM support

[Marc Zyngier]

This series is aimed at offering the minimum level of support that KVM
needs when it comes to architected timers.

The first patch changes the driver to use the virtual timer by default
instead of the physical one (access to the later is conditioned by the
hypervisor settings, while the former is always available). This could
be required even if the kernel is not running as a guest.

The second patch exposes a clockcycle counter that can be used by
other subsystems.

Patches based on v3.6-rc1.


Marc Zyngier (2):
  ARM: arch_timers: enable the use of the virtual timer
  ARM: arch_timers: register a time/cycle counter

 arch/arm/include/asm/arch_timer.h |    7 +
 arch/arm/kernel/arch_timer.c      |  286 ++++++++++++++++++++++++++++--------
 2 files changed, 229 insertions(+), 64 deletions(-)

-- 
1.7.8.6

[PATCH v2 1/2] ARM: arch_timers: enable the use of the virtual timer

[Marc Zyngier]

At the moment, the arch_timer driver only uses the physical timer,
which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
which is likely in a virtualized environment. Instead, the virtual
timer is always available.

This patch enables the use of both the virtual timer, unless no
interrupt is provided in the DT for it, in which case is falls
back to the physical timer.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/arch_timer.c |  261 +++++++++++++++++++++++++++++++----------
 1 files changed, 197 insertions(+), 64 deletions(-)

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index cf25880..4a64733 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -27,13 +27,31 @@
 #include <asm/sched_clock.h>
 
 static unsigned long arch_timer_rate;
-static int arch_timer_ppi;
-static int arch_timer_ppi2;
+
+enum ppi_nr {
+	PHYS_SECURE_PPI,
+	PHYS_NONSECURE_PPI,
+	VIRT_PPI,
+	HYP_PPI,
+	MAX_TIMER_PPI
+};
+
+static int arch_timer_ppi[MAX_TIMER_PPI];
 
 static struct clock_event_device __percpu **arch_timer_evt;
 
 extern void init_current_timer_delay(unsigned long freq);
 
+static bool arch_timer_use_virtual = true;
+
+struct arch_timer_reg_ops {
+	void	(*reg_write)(int reg, u32 val);
+	u32	(*reg_read)(int reg);
+	cycle_t	(*counter_read)(void);
+};
+
+static struct arch_timer_reg_ops *arch_timer_reg_ops;
+
 /*
  * Architected system timer support.
  */
@@ -46,7 +64,37 @@ extern void init_current_timer_delay(unsigned long freq);
 #define ARCH_TIMER_REG_FREQ		1
 #define ARCH_TIMER_REG_TVAL		2
 
-static void arch_timer_reg_write(int reg, u32 val)
+static inline void arch_timer_reg_write(int reg, u32 val)
+{
+	arch_timer_reg_ops->reg_write(reg, val);
+}
+
+static inline u32 arch_timer_reg_read(int reg)
+{
+	return arch_timer_reg_ops->reg_read(reg);
+}
+
+static inline cycle_t arch_timer_counter_read(void)
+{
+	return arch_timer_reg_ops->counter_read();
+}
+
+static u32 arch_timer_common_reg_read(int reg)
+{
+	u32 val;
+
+	switch (reg) {
+	case ARCH_TIMER_REG_FREQ:
+		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
+		break;
+	default:
+		BUG();
+	}
+
+	return val;
+}
+
+static void arch_timer_phys_reg_write(int reg, u32 val)
 {
 	switch (reg) {
 	case ARCH_TIMER_REG_CTRL:
@@ -60,7 +108,7 @@ static void arch_timer_reg_write(int reg, u32 val)
 	isb();
 }
 
-static u32 arch_timer_reg_read(int reg)
+static u32 arch_timer_phys_reg_read(int reg)
 {
 	u32 val;
 
@@ -68,19 +116,78 @@ static u32 arch_timer_reg_read(int reg)
 	case ARCH_TIMER_REG_CTRL:
 		asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
 		break;
-	case ARCH_TIMER_REG_FREQ:
-		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
-		break;
 	case ARCH_TIMER_REG_TVAL:
 		asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
 		break;
 	default:
-		BUG();
+		val = arch_timer_common_reg_read(reg);
+	}
+
+	return val;
+}
+
+static inline cycle_t arch_counter_get_cntpct(void)
+{
+	u32 cvall, cvalh;
+
+	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
+
+	return ((cycle_t) cvalh << 32) | cvall;
+}
+
+static struct arch_timer_reg_ops arch_timer_phys_ops = {
+	.reg_write	= arch_timer_phys_reg_write,
+	.reg_read	= arch_timer_phys_reg_read,
+	.counter_read	= arch_counter_get_cntpct,
+};
+
+static void arch_timer_virt_reg_write(int reg, u32 val)
+{
+	switch (reg) {
+	case ARCH_TIMER_REG_CTRL:
+		asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val));
+		break;
+	case ARCH_TIMER_REG_TVAL:
+		asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val));
+		break;
+	}
+
+	isb();
+}
+
+static u32 arch_timer_virt_reg_read(int reg)
+{
+	u32 val;
+
+	switch (reg) {
+	case ARCH_TIMER_REG_CTRL:
+		asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
+		break;
+	case ARCH_TIMER_REG_TVAL:
+		asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
+		break;
+	default:
+		val = arch_timer_common_reg_read(reg);
 	}
 
 	return val;
 }
 
+static inline cycle_t arch_counter_get_cntvct(void)
+{
+	u32 cvall, cvalh;
+
+	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
+
+	return ((cycle_t) cvalh << 32) | cvall;
+}
+
+static struct arch_timer_reg_ops arch_timer_virt_ops = {
+	.reg_write	= arch_timer_virt_reg_write,
+	.reg_read	= arch_timer_virt_reg_read,
+	.counter_read	= arch_counter_get_cntvct,
+};
+
 static irqreturn_t arch_timer_handler(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
@@ -144,16 +251,19 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 	clk->rating = 450;
 	clk->set_mode = arch_timer_set_mode;
 	clk->set_next_event = arch_timer_set_next_event;
-	clk->irq = arch_timer_ppi;
+	clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
 
 	clockevents_config_and_register(clk, arch_timer_rate,
 					0xf, 0x7fffffff);
 
 	*__this_cpu_ptr(arch_timer_evt) = clk;
 
-	enable_percpu_irq(clk->irq, 0);
-	if (arch_timer_ppi2)
-		enable_percpu_irq(arch_timer_ppi2, 0);
+	if (arch_timer_use_virtual)
+		enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
+	else {
+		enable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], 0);
+		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
+	}
 
 	return 0;
 }
@@ -185,43 +295,30 @@ static int arch_timer_available(void)
 		arch_timer_rate = freq;
 	}
 
-	pr_info_once("Architected local timer running at %lu.%02luMHz.\n",
-		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100);
+	pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
+		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100,
+		     arch_timer_use_virtual ? "virt" : "phys");
 	return 0;
 }
 
-static inline cycle_t arch_counter_get_cntpct(void)
-{
-	u32 cvall, cvalh;
-
-	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
-
-	return ((cycle_t) cvalh << 32) | cvall;
-}
-
-static inline cycle_t arch_counter_get_cntvct(void)
+static u32 notrace arch_counter_get_cnt32(void)
 {
-	u32 cvall, cvalh;
-
-	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
-
-	return ((cycle_t) cvalh << 32) | cvall;
-}
-
-static u32 notrace arch_counter_get_cntvct32(void)
-{
-	cycle_t cntvct = arch_counter_get_cntvct();
+	cycle_t cnt = arch_timer_counter_read();
 
 	/*
 	 * The sched_clock infrastructure only knows about counters
 	 * with@most 32bits. Forget about the upper 24 bits for the
 	 * time being...
 	 */
-	return (u32)(cntvct & (u32)~0);
+	return (u32)(cnt & (u32)~0);
 }
 
 static cycle_t arch_counter_read(struct clocksource *cs)
 {
+	/*
+	 * Always use the physical counter for the clocksource.
+	 * CNTHCTL.PL1PCTEN must be set to 1.
+	 */
 	return arch_counter_get_cntpct();
 }
 
@@ -245,9 +342,14 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 {
 	pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
 		 clk->irq, smp_processor_id());
-	disable_percpu_irq(clk->irq);
-	if (arch_timer_ppi2)
-		disable_percpu_irq(arch_timer_ppi2);
+
+	if (arch_timer_use_virtual)
+		disable_percpu_irq(arch_timer_ppi[VIRT_PPI]);
+	else {
+		disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]);
+		disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
+	}
+
 	arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
@@ -261,36 +363,49 @@ static struct clock_event_device arch_timer_global_evt;
 static int __init arch_timer_register(void)
 {
 	int err;
+	int ppi;
+
+	if (arch_timer_use_virtual)
+		arch_timer_reg_ops = &arch_timer_virt_ops;
+	else
+		arch_timer_reg_ops = &arch_timer_phys_ops;
 
 	err = arch_timer_available();
 	if (err)
-		return err;
+		goto out;
 
 	arch_timer_evt = alloc_percpu(struct clock_event_device *);
-	if (!arch_timer_evt)
-		return -ENOMEM;
+	if (!arch_timer_evt) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
 
-	err = request_percpu_irq(arch_timer_ppi, arch_timer_handler,
-				 "arch_timer", arch_timer_evt);
+	if (arch_timer_use_virtual) {
+		ppi = arch_timer_ppi[VIRT_PPI];
+		err = request_percpu_irq(ppi, arch_timer_handler,
+					 "arch_timer", arch_timer_evt);
+	} else {
+		ppi = arch_timer_ppi[PHYS_SECURE_PPI];
+		err = request_percpu_irq(ppi, arch_timer_handler,
+					 "arch_timer", arch_timer_evt);
+		if (!err) {
+			ppi = arch_timer_ppi[PHYS_NONSECURE_PPI];
+			err = request_percpu_irq(ppi, arch_timer_handler,
+						 "arch_timer", arch_timer_evt);
+			if (err)
+				free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
+						arch_timer_evt);
+		}
+	}
+
 	if (err) {
 		pr_err("arch_timer: can't register interrupt %d (%d)\n",
-		       arch_timer_ppi, err);
+		       ppi, err);
 		goto out_free;
 	}
 
-	if (arch_timer_ppi2) {
-		err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler,
-					 "arch_timer", arch_timer_evt);
-		if (err) {
-			pr_err("arch_timer: can't register interrupt %d (%d)\n",
-			       arch_timer_ppi2, err);
-			arch_timer_ppi2 = 0;
-			goto out_free_irq;
-		}
-	}
-
 	err = local_timer_register(&arch_timer_ops);
 	if (err) {
 		/*
@@ -310,13 +425,18 @@ static int __init arch_timer_register(void)
 	return 0;
 
 out_free_irq:
-	free_percpu_irq(arch_timer_ppi, arch_timer_evt);
-	if (arch_timer_ppi2)
-		free_percpu_irq(arch_timer_ppi2, arch_timer_evt);
+	if (arch_timer_use_virtual)
+		free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
+	else {
+		free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
+				arch_timer_evt);
+		free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
+				arch_timer_evt);
+	}
 
 out_free:
 	free_percpu(arch_timer_evt);
-
+out:
 	return err;
 }
 
@@ -329,6 +449,7 @@ int __init arch_timer_of_register(void)
 {
 	struct device_node *np;
 	u32 freq;
+	int i;
 
 	np = of_find_matching_node(NULL, arch_timer_of_match);
 	if (!np) {
@@ -340,10 +461,22 @@ int __init arch_timer_of_register(void)
 	if (!of_property_read_u32(np, "clock-frequency", &freq))
 		arch_timer_rate = freq;
 
-	arch_timer_ppi = irq_of_parse_and_map(np, 0);
-	arch_timer_ppi2 = irq_of_parse_and_map(np, 1);
-	pr_info("arch_timer: found %s irqs %d %d\n",
-		np->name, arch_timer_ppi, arch_timer_ppi2);
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+
+	/*
+	 * If no interrupt provided for virtual timer, we'll have to
+	 * stick to the physical timer. It'd better be accessible...
+	 */
+	if (!arch_timer_ppi[VIRT_PPI]) {
+		arch_timer_use_virtual = false;
+
+		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
+		    !arch_timer_ppi[PHYS_NONSECURE_PPI]) {
+			pr_warn("arch_timer: No interrupt available, giving up\n");
+			return -EINVAL;
+		}
+	}
 
 	return arch_timer_register();
 }
@@ -356,6 +489,6 @@ int __init arch_timer_sched_clock_init(void)
 	if (err)
 		return err;
 
-	setup_sched_clock(arch_counter_get_cntvct32, 32, arch_timer_rate);
+	setup_sched_clock(arch_counter_get_cnt32, 32, arch_timer_rate);
 	return 0;
 }
-- 
1.7.8.6

[PATCH v2 1/2] ARM: arch_timers: enable the use of the virtual timer

[Cyril Chemparathy]

Marc,

On 8/11/2012 6:31 AM, Marc Zyngier wrote:
> At the moment, the arch_timer driver only uses the physical timer,
> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
> which is likely in a virtualized environment. Instead, the virtual
> timer is always available.
>
> This patch enables the use of both the virtual timer, unless no
> interrupt is provided in the DT for it, in which case is falls
> back to the physical timer.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm/kernel/arch_timer.c |  261 +++++++++++++++++++++++++++++++----------
>   1 files changed, 197 insertions(+), 64 deletions(-)
>
> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index cf25880..4a64733 100644
[...]
> +	if (arch_timer_use_virtual) {
> +		ppi = arch_timer_ppi[VIRT_PPI];
> +		err = request_percpu_irq(ppi, arch_timer_handler,
> +					 "arch_timer", arch_timer_evt);
> +	} else {
> +		ppi = arch_timer_ppi[PHYS_SECURE_PPI];
> +		err = request_percpu_irq(ppi, arch_timer_handler,
> +					 "arch_timer", arch_timer_evt);
> +		if (!err) {
> +			ppi = arch_timer_ppi[PHYS_NONSECURE_PPI];
> +			err = request_percpu_irq(ppi, arch_timer_handler,
> +						 "arch_timer", arch_timer_evt);
> +			if (err)
> +				free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
> +						arch_timer_evt);
> +		}
> +	}
> +
>   	if (err) {
>   		pr_err("arch_timer: can't register interrupt %d (%d)\n",
> -		       arch_timer_ppi, err);
> +		       ppi, err);
>   		goto out_free;
>   	}
>
> -	if (arch_timer_ppi2) {
> -		err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler,
> -					 "arch_timer", arch_timer_evt);
> -		if (err) {
> -			pr_err("arch_timer: can't register interrupt %d (%d)\n",
> -			       arch_timer_ppi2, err);
> -			arch_timer_ppi2 = 0;
> -			goto out_free_irq;
> -		}
> -	}
> -

In the original code, arch_timer_ppi2 appears to have been entirely 
optional.

With this change, the code appears to mandate that both nonsecure and 
secure PPIs be available (if !arch_timer_use_virtual).  Could you please 
explain this change?

-- 
Thanks
- Cyril

[PATCH v2 1/2] ARM: arch_timers: enable the use of the virtual timer

[Cyril Chemparathy]

On 8/11/2012 6:31 AM, Marc Zyngier wrote:
> At the moment, the arch_timer driver only uses the physical timer,
> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
> which is likely in a virtualized environment. Instead, the virtual
> timer is always available.
>
> This patch enables the use of both the virtual timer, unless no
> interrupt is provided in the DT for it, in which case is falls
> back to the physical timer.
>

I'm curious about the cost of the added pointer chasing introduced by 
this patch.

The original code gets nicely inlined by the compiler, and this hides 
all the switch-case register index stuff.  For instance:

  10797 c001288c <arch_timer_set_next_event>:
  10798 c001288c:       ee1e3f32        mrc     15, 0, r3, cr14, cr2, {1}
  10799 c0012890:       e3c33002        bic     r3, r3, #2
  10800 c0012894:       ee0e0f12        mcr     15, 0, r0, cr14, cr2, {0}
  10801 c0012898:       f57ff06f        isb     sy
  10802 c001289c:       e3833001        orr     r3, r3, #1
  10803 c00128a0:       ee0e3f32        mcr     15, 0, r3, cr14, cr2, {1}
  10804 c00128a4:       f57ff06f        isb     sy
  10805 c00128a8:       e3a00000        mov     r0, #0
  10806 c00128ac:       e12fff1e        bx      lr

With the added pointer chasing, we unfortunately lose out on all the 
work that the compiler used to do for us.  We now end up having to snake 
our way through the following:

  10852 c0012958 <arch_timer_set_next_event>:
  10853 c0012958:       e92d4070        push    {r4, r5, r6, lr}
  10854 c001295c:       e3054400        movw    r4, #21504      ; 0x5400
  10855 c0012960:       e34c401f        movt    r4, #49183      ; 0xc01f
  10856 c0012964:       e1a06000        mov     r6, r0
  10857 c0012968:       e3a00000        mov     r0, #0
  10858 c001296c:       e5943000        ldr     r3, [r4]
  10859 c0012970:       e5933004        ldr     r3, [r3, #4]
  10860 c0012974:       e12fff33        blx     r3
  10861 c0012978:       e5943000        ldr     r3, [r4]
  10862 c001297c:       e3c05002        bic     r5, r0, #2
  10863 c0012980:       e1a01006        mov     r1, r6
  10864 c0012984:       e3a00002        mov     r0, #2
  10865 c0012988:       e5933000        ldr     r3, [r3]
  10866 c001298c:       e12fff33        blx     r3
  10867 c0012990:       e5943000        ldr     r3, [r4]
  10868 c0012994:       e3851001        orr     r1, r5, #1
  10869 c0012998:       e3a00000        mov     r0, #0
  10870 c001299c:       e5933000        ldr     r3, [r3]
  10871 c00129a0:       e12fff33        blx     r3
  10872 c00129a4:       e3a00000        mov     r0, #0
  10873 c00129a8:       e8bd8070        pop     {r4, r5, r6, pc}

  10908 c0012a18 <arch_timer_phys_reg_read>:
  10909 c0012a18:       e3500000        cmp     r0, #0
  10910 c0012a1c:       e92d4008        push    {r3, lr}
  10911 c0012a20:       1a000001        bne     c0012a2c
  10912 c0012a24:       ee1e0f32        mrc     15, 0, r0, cr14, cr2, {1}
  10913 c0012a28:       e8bd8008        pop     {r3, pc}
  10914 c0012a2c:       e3500002        cmp     r0, #2
  10915 c0012a30:       0a000002        beq     c0012a40
  10916 c0012a34:       e3500001        cmp     r0, #1
  10917 c0012a38:       0a000002        beq     c0012a48
  10918 c0012a3c:       eb053afb        bl      c0161630
  10919 c0012a40:       ee1e0f12        mrc     15, 0, r0, cr14, cr2, {0}
  10920 c0012a44:       e8bd8008        pop     {r3, pc}
  10921 c0012a48:       ee1e0f10        mrc     15, 0, r0, cr14, cr0, {0}
  10922 c0012a4c:       e8bd8008        pop     {r3, pc}

  10768 c0012840 <arch_timer_phys_reg_write>:
  10769 c0012840:       e3500000        cmp     r0, #0
  10770 c0012844:       1a000002        bne     c0012854
  10771 c0012848:       ee0e1f32        mcr     15, 0, r1, cr14, cr2, {1}
  10772 c001284c:       f57ff06f        isb     sy
  10773 c0012850:       e12fff1e        bx      lr
  10774 c0012854:       e3500002        cmp     r0, #2
  10775 c0012858:       1afffffb        bne     c001284c
  10776 c001285c:       ee0e1f12        mcr     15, 0, r1, cr14, cr2, {0}
  10777 c0012860:       f57ff06f        isb     sy
  10778 c0012864:       e12fff1e        bx      lr


I think we'd be better off separating between these (virt, phys, ...) 
implementations at a higher level of operations (set_mode, 
set_next_event, ...) rather than separating at a register operations 
level as you have in this patch.

-- 
Thanks
- Cyril

[PATCH v2 1/2] ARM: arch_timers: enable the use of the virtual timer

[Marc Zyngier]

On 11/08/12 15:28, Cyril Chemparathy wrote:
> On 8/11/2012 6:31 AM, Marc Zyngier wrote:
>> At the moment, the arch_timer driver only uses the physical timer,
>> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
>> which is likely in a virtualized environment. Instead, the virtual
>> timer is always available.
>>
>> This patch enables the use of both the virtual timer, unless no
>> interrupt is provided in the DT for it, in which case is falls
>> back to the physical timer.
>>
> 
> I'm curious about the cost of the added pointer chasing introduced by 
> this patch.
> 
> The original code gets nicely inlined by the compiler, and this hides 
> all the switch-case register index stuff.  For instance:
> 
>   10797 c001288c <arch_timer_set_next_event>:
>   10798 c001288c:       ee1e3f32        mrc     15, 0, r3, cr14, cr2, {1}
>   10799 c0012890:       e3c33002        bic     r3, r3, #2
>   10800 c0012894:       ee0e0f12        mcr     15, 0, r0, cr14, cr2, {0}
>   10801 c0012898:       f57ff06f        isb     sy
>   10802 c001289c:       e3833001        orr     r3, r3, #1
>   10803 c00128a0:       ee0e3f32        mcr     15, 0, r3, cr14, cr2, {1}
>   10804 c00128a4:       f57ff06f        isb     sy
>   10805 c00128a8:       e3a00000        mov     r0, #0
>   10806 c00128ac:       e12fff1e        bx      lr
> 
> With the added pointer chasing, we unfortunately lose out on all the 
> work that the compiler used to do for us.  We now end up having to snake 
> our way through the following:

[...]

Yup, that doesn't look too good.

> I think we'd be better off separating between these (virt, phys, ...) 
> implementations at a higher level of operations (set_mode, 
> set_next_event, ...) rather than separating at a register operations 
> level as you have in this patch.

This approach leads to a lot of code duplication, unless we do some ugly
preprocessor hackery (see patch attached). I then get much better code
(smaller, faster), but I'm not sure we want to live with this...

	M.
-- 
Jazz is not dead. It just smells funny...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ARM-arch_timers-enable-the-use-of-the-virtual-timer.patch
Type: text/x-diff
Size: 14514 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120813/b73ddd01/attachment-0001.bin>

[PATCH v2 2/2] ARM: arch_timers: register a time/cycle counter

[Marc Zyngier]

Some subsystems (KVM for example) need access to a cycle counter.
In the KVM case, this is used to measure the time delta between
host and guest in order to accurately generate timer events for
the guest.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/arch_timer.h |    7 +++++++
 arch/arm/kernel/arch_timer.c      |   25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 62e7547..ad9b155 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -2,11 +2,13 @@
 #define __ASMARM_ARCH_TIMER_H
 
 #include <asm/errno.h>
+#include <linux/clocksource.h>
 
 #ifdef CONFIG_ARM_ARCH_TIMER
 #define ARCH_HAS_READ_CURRENT_TIMER
 int arch_timer_of_register(void);
 int arch_timer_sched_clock_init(void);
+struct timecounter *arch_timer_get_timecounter(void);
 #else
 static inline int arch_timer_of_register(void)
 {
@@ -17,6 +19,11 @@ static inline int arch_timer_sched_clock_init(void)
 {
 	return -ENXIO;
 }
+
+static inline struct timecounter *arch_timer_get_timecounter(void)
+{
+	return NULL;
+}
 #endif
 
 #endif
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index 4a64733..f57e4b7 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -330,6 +330,15 @@ int read_current_timer(unsigned long *timer_val)
 	return 0;
 }
 
+static cycle_t arch_counter_read_cc(const struct cyclecounter *cc)
+{
+	/*
+	 * Always use the physical counter for the clocksource.
+	 * CNTHCTL.PL1PCTEN must be set to 1.
+	 */
+	return arch_counter_get_cntpct();
+}
+
 static struct clocksource clocksource_counter = {
 	.name	= "arch_sys_counter",
 	.rating	= 400,
@@ -338,6 +347,18 @@ static struct clocksource clocksource_counter = {
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
+static struct cyclecounter cyclecounter = {
+	.read	= arch_counter_read_cc,
+	.mask	= CLOCKSOURCE_MASK(56),
+};
+
+static struct timecounter timecounter;
+
+struct timecounter *arch_timer_get_timecounter(void)
+{
+	return &timecounter;
+}
+
 static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 {
 	pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
@@ -381,6 +402,10 @@ static int __init arch_timer_register(void)
 	}
 
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
+	cyclecounter.mult = clocksource_counter.mult;
+	cyclecounter.shift = clocksource_counter.shift;
+	timecounter_init(&timecounter, &cyclecounter,
+			 arch_counter_get_cntpct());
 
 	if (arch_timer_use_virtual) {
 		ppi = arch_timer_ppi[VIRT_PPI];
-- 
1.7.8.6