[RFC PATCH 0/4] arch_timers patches to enable KVM support

[RFC PATCH 0/4] 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 shouldn't be controvertial, as it changes the driver
to use the virtual timer instead of the physical one (access to the
latest is conditioned by the hypervisor settings, while the former is
always available). It is actually required if the kernel is run as a
guest. The next two are basic infrastructure additions.

The last one is really the cause of the RFC tag. It allows the kernel
to switch from using the virtual timer to the physical one - KVM needs
this to allow guests to use the virtual timer, and because it runs in
PL2, it allows the kernel to use the physical timer. This allows the
same kernel to be used both as a host or guest kernel.

The ugly part of this patch is that that KVM needs to handle the
virtual timer interrupt itself, and this patch adds some sort of
interrupt offloading. Frankly, I hate it, but I'm posting it so people
can show they have better ideas than I do... ;-)

Patches based on v3.5-rc5.

Marc Zyngier (4):
  ARM: arch_timers: enable the use of the virtual timer
  ARM: arch_timers: register a time/cycle counter
  ARM: arch_timers: give the virtual timer its own interrupt handler
  ARM: arch_timers: dynamic switch to physical timer and virtual timer
    offloading

 arch/arm/include/asm/arch_timer.h |   14 ++
 arch/arm/kernel/arch_timer.c      |  330 ++++++++++++++++++++++++++++++-------
 2 files changed, 284 insertions(+), 60 deletions(-)

-- 
1.7.10.3

[RFC PATCH 1/4] 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 |  248 ++++++++++++++++++++++++++++++++----------
 1 file changed, 188 insertions(+), 60 deletions(-)

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index dd58035..54599e9 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -27,11 +27,29 @@
 #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;
 
+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 __percpu **arch_timer_reg_ops;
+
 /*
  * Architected system timer support.
  */
@@ -44,7 +62,37 @@ static struct clock_event_device __percpu **arch_timer_evt;
 #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)
+{
+	(*__this_cpu_ptr(arch_timer_reg_ops))->reg_write(reg, val);
+}
+
+static inline u32 arch_timer_reg_read(int reg)
+{
+	return (*__this_cpu_ptr(arch_timer_reg_ops))->reg_read(reg);
+}
+
+static inline cycle_t arch_timer_counter_read(void)
+{
+	return (*__this_cpu_ptr(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:
@@ -58,7 +106,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;
 
@@ -66,19 +114,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;
@@ -134,6 +241,8 @@ static int arch_timer_set_next_event(unsigned long evt,
 
 static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 {
+	int i;
+
 	/* Be safe... */
 	arch_timer_disable();
 
@@ -142,16 +251,16 @@ 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);
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		if (arch_timer_ppi[i])
+			enable_percpu_irq(arch_timer_ppi[i], 0);
 
 	return 0;
 }
@@ -188,38 +297,24 @@ static int arch_timer_available(void)
 	return 0;
 }
 
-static inline cycle_t arch_counter_get_cntpct(void)
+static u32 notrace arch_counter_get_cnt32(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)
-{
-	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();
 }
 
@@ -233,11 +328,13 @@ static struct clocksource clocksource_counter = {
 
 static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 {
+	int i;
+
 	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);
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		if (arch_timer_ppi[i])
+			disable_percpu_irq(arch_timer_ppi[i]);
 	arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
@@ -251,33 +348,48 @@ static struct clock_event_device arch_timer_global_evt;
 static int __init arch_timer_register(void)
 {
 	int err;
+	int i;
+	int cpu;
+
+	arch_timer_reg_ops = alloc_percpu(struct arch_timer_reg_ops *);
+	if (!arch_timer_reg_ops)
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu) {
+		struct arch_timer_reg_ops **p;
+
+		p = per_cpu_ptr(arch_timer_reg_ops, cpu);
+		if (arch_timer_use_virtual)
+			*p = &arch_timer_virt_ops;
+		else
+			*p = &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 (err) {
-		pr_err("arch_timer: can't register interrupt %d (%d)\n",
-		       arch_timer_ppi, err);
-		goto out_free;
-	}
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++) {
+		irq_handler_t handler = arch_timer_handler;
 
-	if (arch_timer_ppi2) {
-		err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler,
+		if (!arch_timer_ppi[i])
+			continue;
+
+		err = request_percpu_irq(arch_timer_ppi[i], 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;
+			       arch_timer_ppi[i], err);
+			arch_timer_ppi[i] = 0;
+			goto out_free;
 		}
 	}
 
@@ -299,13 +411,14 @@ 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);
+	for ( ; i > PHYS_SECURE_PPI; i--)
+		if (arch_timer_ppi[i])
+			free_percpu_irq(arch_timer_ppi[i], arch_timer_evt);
 
 out_free:
 	free_percpu(arch_timer_evt);
-
+out:
+	free_percpu(arch_timer_reg_ops);
 	return err;
 }
 
@@ -318,6 +431,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) {
@@ -329,10 +443,24 @@ 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);
+	pr_info("arch_timer: found %s irqs ", np->name);
+
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++) {
+		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+		if (!arch_timer_ppi[i]) {
+			/*
+			 * No interrupt provided for virtual timer,
+			 * we'll have to stick to the physical timer.
+			 */
+			if (i == VIRT_PPI)
+				arch_timer_use_virtual = false;
+			
+			continue;
+		}
+		pr_cont("%d ", arch_timer_ppi[i]);
+	}
+
+	pr_cont("\n");
 
 	return arch_timer_register();
 }
@@ -345,6 +473,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.10.3

[RFC PATCH 1/4] ARM: arch_timers: enable the use of the virtual timer

[Christopher Covington]

Hi Marc,

On 07/06/2012 05:20 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.

[...]

> @@ -44,7 +62,37 @@ static struct clock_event_device __percpu **arch_timer_evt;
>  #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)

Will GCC fail to inline without the hint?

> +{
> +	(*__this_cpu_ptr(arch_timer_reg_ops))->reg_write(reg, val);
> +}
> +
> +static inline u32 arch_timer_reg_read(int reg)
> +{
> +	return (*__this_cpu_ptr(arch_timer_reg_ops))->reg_read(reg);
> +}
> +
> +static inline cycle_t arch_timer_counter_read(void)
> +{
> +	return (*__this_cpu_ptr(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:

[...]

> @@ -329,10 +443,24 @@ 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);
> +	pr_info("arch_timer: found %s irqs ", np->name);
> +
> +	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++) {
> +		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
> +		if (!arch_timer_ppi[i]) {
> +			/*
> +			 * No interrupt provided for virtual timer,
> +			 * we'll have to stick to the physical timer.
> +			 */
> +			if (i == VIRT_PPI)
> +				arch_timer_use_virtual = false;
> +			

It seems like it would be more straightforward to set the
arch_timer_use_virtual variable outside of the loop, directly indexing
into the array of timer PPI's. On a trivial note you've also got
trailing whitespace here.

> +			continue;
> +		}
> +		pr_cont("%d ", arch_timer_ppi[i]);
> +	}
> +
> +	pr_cont("\n");
>  
>  	return arch_timer_register();
>  }
> @@ -345,6 +473,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;
>  }

Regards,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC PATCH 1/4] ARM: arch_timers: enable the use of the virtual timer

[Christopher Covington]

Hi Marc,

Here's one additional small question about your explanation in the
commit message.

On 07/06/2012 05:20 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.

If the virtual timer is "always available", why would a device tree
description of the hardware leave that interrupt number out as described
below?

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

[...]

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC PATCH 2/4] 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 |    8 ++++++++
 arch/arm/kernel/arch_timer.c      |   25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index ed2e95d..72c6822 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -1,9 +1,12 @@
 #ifndef __ASMARM_ARCH_TIMER_H
 #define __ASMARM_ARCH_TIMER_H
 
+#include <linux/clocksource.h>
+
 #ifdef CONFIG_ARM_ARCH_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)
 {
@@ -14,6 +17,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 54599e9..2b5da66 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -318,6 +318,15 @@ static cycle_t arch_counter_read(struct clocksource *cs)
 	return arch_counter_get_cntpct();
 }
 
+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,
@@ -326,6 +335,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)
 {
 	int i;
@@ -376,6 +397,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());
 
 	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++) {
 		irq_handler_t handler = arch_timer_handler;
-- 
1.7.10.3

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

[Christopher Covington]

Hi Marc,

On 07/06/2012 05:20 AM, Marc Zyngier wrote:
> 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.

[...]

> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index 54599e9..2b5da66 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c
> @@ -318,6 +318,15 @@ static cycle_t arch_counter_read(struct clocksource *cs)
>  	return arch_counter_get_cntpct();
>  }
>  
> +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();
> +}

Why not just provide a wrapper around arch_timer_counter_read()? Will a
guest OS never need or want to use the cyclecounter (and clocksource for
that matter) interface?

> +
>  static struct clocksource clocksource_counter = {
>  	.name	= "arch_sys_counter",
>  	.rating	= 400,
> @@ -326,6 +335,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)
>  {
>  	int i;

[...]

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC PATCH 3/4] ARM: arch_timers: give the virtual timer its own interrupt handler

[Marc Zyngier]

In order to prepare for the timer interrupt offloading to the hypervisor,
give the virtual timer its own interrupt handler.

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

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index 2b5da66..4473f66 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -202,6 +202,11 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
+static irqreturn_t arch_timer_virt_handler(int irq, void *dev_id)
+{
+	return arch_timer_handler(irq, dev_id);
+}
+
 static void arch_timer_disable(void)
 {
 	unsigned long ctrl;
@@ -408,6 +413,9 @@ static int __init arch_timer_register(void)
 		if (!arch_timer_ppi[i])
 			continue;
 
+		if (i == VIRT_PPI)
+			handler = arch_timer_virt_handler;
+
 		err = request_percpu_irq(arch_timer_ppi[i], handler,
 					 "arch_timer", arch_timer_evt);
 		if (err) {
-- 
1.7.10.3

[RFC PATCH 3/4] ARM: arch_timers: give the virtual timer its own interrupt handler

[Christopher Covington]

Hi Marc,

On 07/06/2012 05:20 AM, Marc Zyngier wrote:
> In order to prepare for the timer interrupt offloading to the hypervisor,
> give the virtual timer its own interrupt handler.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kernel/arch_timer.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index 2b5da66..4473f66 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c

[...]

> @@ -408,6 +413,9 @@ static int __init arch_timer_register(void)
>  		if (!arch_timer_ppi[i])
>  			continue;
>  
> +		if (i == VIRT_PPI)
> +			handler = arch_timer_virt_handler;
> +

Again, it seems awkward to do this sort of thing inside the loop as
you're only keying off of one value with a known index in the array.

>  		err = request_percpu_irq(arch_timer_ppi[i], handler,
>  					 "arch_timer", arch_timer_evt);
>  		if (err) {

Regards,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC PATCH 4/4] ARM: arch_timers: dynamic switch to physical timer and virtual timer offloading

[Marc Zyngier]

In an environment supporting virtualization (KVM), the virtual timer
is reserved to the guests, while we rely on the physical timer for
the host.

For this, we need to:
- switch the host CPUs from the virtual timer to the physical one
- provide an interrupt handler that is called by the virtual
  timer's interrupt handler.

The new function arch_timer_switch_to_phys() performs this task.

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

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 72c6822..7cd1e27 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 <linux/clocksource.h>
+#include <linux/interrupt.h>
 
 #ifdef CONFIG_ARM_ARCH_TIMER
 int arch_timer_of_register(void);
 int arch_timer_sched_clock_init(void);
 struct timecounter *arch_timer_get_timecounter(void);
+void arch_timer_switch_to_phys(irq_handler_t);
 #else
 static inline int arch_timer_of_register(void)
 {
@@ -22,6 +24,10 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
 {
 	return NULL;
 }
+
+static inline void arch_timer_switch_to_phys(irq_handler_t handler)
+{
+}
 #endif
 
 #endif
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index 4473f66..1bb632a 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -50,6 +50,8 @@ struct arch_timer_reg_ops {
 
 static struct arch_timer_reg_ops __percpu **arch_timer_reg_ops;
 
+static irq_handler_t arch_timer_virt_external_handler;
+
 /*
  * Architected system timer support.
  */
@@ -204,6 +206,19 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
 
 static irqreturn_t arch_timer_virt_handler(int irq, void *dev_id)
 {
+	if (arch_timer_virt_external_handler) {
+		unsigned long ctrl;
+
+		ctrl = arch_timer_virt_reg_read(ARCH_TIMER_REG_CTRL);
+		if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
+			ctrl |= ARCH_TIMER_CTRL_IT_MASK;
+			arch_timer_virt_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
+			return arch_timer_virt_external_handler(irq, NULL);
+		}
+
+		return IRQ_NONE;
+	}
+
 	return arch_timer_handler(irq, dev_id);
 }
 
@@ -509,3 +524,37 @@ int __init arch_timer_sched_clock_init(void)
 	setup_sched_clock(arch_counter_get_cnt32, 32, arch_timer_rate);
 	return 0;
 }
+
+static void arch_timer_switch_cpu_to_phys(void *dummy)
+{
+	u32 cvall, cvalh, val;
+
+	pr_info("Switching CPU%d to physical timer\n", smp_processor_id());
+
+	asm volatile("mrrc p15, 3, %0, %1, c14	\n" /* Read CNTV_CVAL */
+		     "mcrr p15, 2, %0, %1, c14	\n" /* Write CNTP_CVAL */
+		     : "=r" (cvall), "=r" (cvalh));
+
+	isb();
+	
+	val = arch_timer_virt_reg_read(ARCH_TIMER_REG_CTRL);
+	arch_timer_virt_reg_write(ARCH_TIMER_REG_CTRL,
+				  val & ~ARCH_TIMER_CTRL_ENABLE);
+	arch_timer_phys_reg_write(ARCH_TIMER_REG_CTRL, val);
+	*__this_cpu_ptr(arch_timer_reg_ops) = &arch_timer_phys_ops;
+}
+
+void arch_timer_switch_to_phys(irq_handler_t handler)
+{
+	int cpu;
+
+	if (!arch_timer_use_virtual)
+		return;
+
+	for_each_online_cpu(cpu)
+		smp_call_function_single(cpu, arch_timer_switch_cpu_to_phys,
+					 NULL, 1);
+
+	arch_timer_use_virtual = false;
+	arch_timer_virt_external_handler = handler;
+}
-- 
1.7.10.3

[RFC PATCH 4/4] ARM: arch_timers: dynamic switch to physical timer and virtual timer offloading

[Christopher Covington]

Hi Marc,

On 07/06/2012 05:20 AM, Marc Zyngier wrote:
> In an environment supporting virtualization (KVM), the virtual timer
> is reserved to the guests, while we rely on the physical timer for
> the host.
> 
> For this, we need to:
> - switch the host CPUs from the virtual timer to the physical one
> - provide an interrupt handler that is called by the virtual
>   timer's interrupt handler.
> 
> The new function arch_timer_switch_to_phys() performs this task.

It might be useful to CC the KVM mailing list to get their feedback.

I feel like it's hard to give useful feedback without the context of the
calling code. If the calling code is some kind of virtual clockchip,
then could you use the existing clock event distribution mechanism
rather than the custom hook?

[...]

> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index 4473f66..1bb632a 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c
> @@ -50,6 +50,8 @@ struct arch_timer_reg_ops {
>  
>  static struct arch_timer_reg_ops __percpu **arch_timer_reg_ops;
>  
> +static irq_handler_t arch_timer_virt_external_handler;
> +
>  /*
>   * Architected system timer support.
>   */
> @@ -204,6 +206,19 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
>  
>  static irqreturn_t arch_timer_virt_handler(int irq, void *dev_id)
>  {
> +	if (arch_timer_virt_external_handler) {
> +		unsigned long ctrl;
> +
> +		ctrl = arch_timer_virt_reg_read(ARCH_TIMER_REG_CTRL);
> +		if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
> +			ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> +			arch_timer_virt_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +			return arch_timer_virt_external_handler(irq, NULL);
> +		}
> +
> +		return IRQ_NONE;
> +	}
> +
>  	return arch_timer_handler(irq, dev_id);
>  }

Perhaps you could avoid some code duplication by calling
arch_timer_handler at the beginning, stashing its return value, calling
the external hook if it's defined, then returning whichever return value
is appropriate.

>  
> @@ -509,3 +524,37 @@ int __init arch_timer_sched_clock_init(void)
>  	setup_sched_clock(arch_counter_get_cnt32, 32, arch_timer_rate);
>  	return 0;
>  }
> +
> +static void arch_timer_switch_cpu_to_phys(void *dummy)
> +{
> +	u32 cvall, cvalh, val;
> +
> +	pr_info("Switching CPU%d to physical timer\n", smp_processor_id());
> +
> +	asm volatile("mrrc p15, 3, %0, %1, c14	\n" /* Read CNTV_CVAL */
> +		     "mcrr p15, 2, %0, %1, c14	\n" /* Write CNTP_CVAL */
> +		     : "=r" (cvall), "=r" (cvalh));

I take it you don't use the easily readable helpers here because you're
afraid of losing a few ticks? That makes me wonder if the
mrc/mcr/mrrc/mcrr helpers could be expanded and improved to make them
usable under these circumstances.

> +
> +	isb();
> +	

(Trailing whitespace)

> +	val = arch_timer_virt_reg_read(ARCH_TIMER_REG_CTRL);
> +	arch_timer_virt_reg_write(ARCH_TIMER_REG_CTRL,
> +				  val & ~ARCH_TIMER_CTRL_ENABLE);
> +	arch_timer_phys_reg_write(ARCH_TIMER_REG_CTRL, val);
> +	*__this_cpu_ptr(arch_timer_reg_ops) = &arch_timer_phys_ops;
> +}
> +
> +void arch_timer_switch_to_phys(irq_handler_t handler)
> +{
> +	int cpu;
> +
> +	if (!arch_timer_use_virtual)
> +		return;

What will call this function? Won't it want to know the call failed?

> +
> +	for_each_online_cpu(cpu)
> +		smp_call_function_single(cpu, arch_timer_switch_cpu_to_phys,
> +					 NULL, 1);
> +
> +	arch_timer_use_virtual = false;
> +	arch_timer_virt_external_handler = handler;
> +}

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum