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

From: [email-masked] (Cyril Chemparathy)
Date: Thu, 30 Aug 2012 14:28:17 -0400

Hi Marc,

On 8/24/2012 10:52 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 the virtual timer, unless no
> interrupt is provided in the DT for it, in which case it falls
> back to the physical timer.
>
> Signed-off-by: Marc Zyngier <[email-masked]>

Looks very nice...  Minor comments below.

> ---
>   arch/arm/kernel/arch_timer.c | 337 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 233 insertions(+), 104 deletions(-)
>
> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index cf25880..3f63b90 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c
> @@ -27,13 +27,23 @@
>   #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;
> +
>   /*
>    * Architected system timer support.
>    */
> @@ -43,53 +53,98 @@ extern void init_current_timer_delay(unsigned long freq);
>   #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
>
>   #define ARCH_TIMER_REG_CTRL		0
> -#define ARCH_TIMER_REG_FREQ		1
> -#define ARCH_TIMER_REG_TVAL		2
> +#define ARCH_TIMER_REG_TVAL		1
>
> -static void arch_timer_reg_write(int reg, u32 val)
> +#define ARCH_TIMER_PHYS_ACCESS		0
> +#define ARCH_TIMER_VIRT_ACCESS		1
> +
> +/*
> + * These register accessors are marked inline so the compiler can
> + * nicely work out which register we want, and chuck away the rest of
> + * the code. At least it does so with a recent GCC (4.6.3).
> + */
> +static inline void arch_timer_reg_write(const int access, const int reg, u32 val)
>   {
> -	switch (reg) {
> -	case ARCH_TIMER_REG_CTRL:
> -		asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
> -		break;
> -	case ARCH_TIMER_REG_TVAL:
> -		asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
> -		break;
> +	if (access == ARCH_TIMER_PHYS_ACCESS) {
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
> +			break;
> +		}
> +	}
> +
> +	if (access == ARCH_TIMER_VIRT_ACCESS) {
> +		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_reg_read(int reg)
> +static inline u32 arch_timer_reg_read(const int access, const int reg)
>   {
> -	u32 val;
> +	u32 val = 0;
> +
> +	if (access == ARCH_TIMER_PHYS_ACCESS) {
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> +			break;
> +		}
> +	}
>
> -	switch (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();
> +	if (access == ARCH_TIMER_VIRT_ACCESS) {
> +		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;
> +		}
>   	}
>
>   	return val;
>   }
>
> -static irqreturn_t arch_timer_handler(int irq, void *dev_id)
> +static inline cycle_t arch_counter_get_cntpct(void)
>   {
> -	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
> -	unsigned long ctrl;
> +	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;
> +}

We should use the Q and R qualifiers to avoid compiler misbehavior:
	u64 cval;
	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cvall));

The compiler generates sad looking code when constructing 64-bit 
quantities with shifts and ORs.  We found this while implementing the 
phys-virt patching for 64-bit phys_addr_t.

Is there value in unifying the physical and virtual counter read 
functions using the access specifier as you've done for the register 
read and write functions?

>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> +static irqreturn_t inline timer_handler(const int access,
> +					struct clock_event_device *evt)
> +{
> +	unsigned long ctrl;
> +	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
>   	if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
>   		ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> -		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
>   		evt->event_handler(evt);
>   		return IRQ_HANDLED;
>   	}
> @@ -97,63 +152,100 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
>   	return IRQ_NONE;
>   }
>
> -static void arch_timer_disable(void)
> +static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id)
>   {
> -	unsigned long ctrl;
> +	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> -	ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
> -	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +	return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt);
>   }
>
> -static void arch_timer_set_mode(enum clock_event_mode mode,
> -				struct clock_event_device *clk)
> +static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
>   {
> +	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
> +
> +	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
> +}
> +
> +static inline void timer_set_mode(const int access, int mode)
> +{
> +	unsigned long ctrl;
>   	switch (mode) {
>   	case CLOCK_EVT_MODE_UNUSED:
>   	case CLOCK_EVT_MODE_SHUTDOWN:
> -		arch_timer_disable();
> +		ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
> +		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
> +		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
>   		break;
>   	default:
>   		break;
>   	}
>   }
>
> -static int arch_timer_set_next_event(unsigned long evt,
> -				     struct clock_event_device *unused)
> +static void arch_timer_set_mode_virt(enum clock_event_mode mode,
> +				     struct clock_event_device *clk)
>   {
> -	unsigned long ctrl;
> +	timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode);
> +}
> +
> +static void arch_timer_set_mode_phys(enum clock_event_mode mode,
> +				     struct clock_event_device *clk)
> +{
> +	timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode);
> +}
>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> +static inline void set_next_event(const int access, unsigned long evt)
> +{
> +	unsigned long ctrl;
> +	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
>   	ctrl |= ARCH_TIMER_CTRL_ENABLE;
>   	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> +	arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt);
> +	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
> +}
>
> -	arch_timer_reg_write(ARCH_TIMER_REG_TVAL, evt);
> -	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +static int arch_timer_set_next_event_virt(unsigned long evt,
> +					  struct clock_event_device *unused)
> +{
> +	set_next_event(ARCH_TIMER_VIRT_ACCESS, evt);
> +	return 0;
> +}
>
> +static int arch_timer_set_next_event_phys(unsigned long evt,
> +					  struct clock_event_device *unused)
> +{
> +	set_next_event(ARCH_TIMER_PHYS_ACCESS, evt);
>   	return 0;
>   }
>
>   static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
>   {
> -	/* Be safe... */
> -	arch_timer_disable();
> -
>   	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
>   	clk->name = "arch_sys_timer";
>   	clk->rating = 450;
> -	clk->set_mode = arch_timer_set_mode;
> -	clk->set_next_event = arch_timer_set_next_event;
> -	clk->irq = arch_timer_ppi;
> +	if (arch_timer_use_virtual) {
> +		arch_timer_set_mode_virt(CLOCK_EVT_MODE_SHUTDOWN, NULL);
> +		clk->set_mode = arch_timer_set_mode_virt;
> +		clk->set_next_event = arch_timer_set_next_event_virt;
> +	} else {
> +		arch_timer_set_mode_phys(CLOCK_EVT_MODE_SHUTDOWN, NULL);
> +		clk->set_mode = arch_timer_set_mode_phys;
> +		clk->set_next_event = arch_timer_set_next_event_phys;
> +	}
> +		
> +	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);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> +	}
>
>   	return 0;
>   }
> @@ -173,8 +265,7 @@ static int arch_timer_available(void)
>   		return -ENXIO;
>
>   	if (arch_timer_rate == 0) {
> -		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, 0);
> -		freq = arch_timer_reg_read(ARCH_TIMER_REG_FREQ);
> +		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (freq));
>
>   		/* Check the timer frequency. */
>   		if (freq == 0) {
> @@ -185,43 +276,42 @@ 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)
> +static u32 notrace arch_counter_get_cntpct32(void)
>   {
> -	u32 cvall, cvalh;
> +	cycle_t cnt = arch_counter_get_cntpct();
>
> -	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;
> +	/*
> +	 * The sched_clock infrastructure only knows about counters
> +	 * with at most 32bits. Forget about the upper 24 bits for the
> +	 * time being...
> +	 */
> +	return (u32)(cnt & (u32)~0);

Wouldn't a return (u32)cnt be sufficient here?

>   }
>
>   static u32 notrace arch_counter_get_cntvct32(void)
>   {
> -	cycle_t cntvct = arch_counter_get_cntvct();
> +	cycle_t cnt = arch_counter_get_cntvct();
>
>   	/*
>   	 * The sched_clock infrastructure only knows about counters
>   	 * with at 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,10 +335,16 @@ 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);
> -	arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
> +
> +	if (arch_timer_use_virtual) {
> +		disable_percpu_irq(arch_timer_ppi[VIRT_PPI]);
> +		arch_timer_set_mode_virt(CLOCK_EVT_MODE_UNUSED, clk);
> +	} else {
> +		disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> +		arch_timer_set_mode_phys(CLOCK_EVT_MODE_UNUSED, clk);
> +	}
>   }
>
>   static struct local_timer_ops arch_timer_ops __cpuinitdata = {
> @@ -261,36 +357,44 @@ static struct clock_event_device arch_timer_global_evt;
>   static int __init arch_timer_register(void)
>   {
>   	int err;
> +	int ppi;
>
>   	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_virt,
> +					 "arch_timer", arch_timer_evt);
> +	} else {
> +		ppi = arch_timer_ppi[PHYS_SECURE_PPI];
> +		err = request_percpu_irq(ppi, arch_timer_handler_phys,
> +					 "arch_timer", arch_timer_evt);
> +		if (!err && arch_timer_ppi[PHYS_NONSECURE_PPI]) {
> +			ppi = arch_timer_ppi[PHYS_NONSECURE_PPI];
> +			err = request_percpu_irq(ppi, arch_timer_handler_phys,
> +						 "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 +414,19 @@ 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);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
> +					arch_timer_evt);
> +	}
>
>   out_free:
>   	free_percpu(arch_timer_evt);
> -
> +out:
>   	return err;
>   }
>
> @@ -329,6 +439,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,22 +451,40 @@ 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();
>   }
>
>   int __init arch_timer_sched_clock_init(void)
>   {
> +	u32 (*cnt32)(void);
>   	int err;
>
>   	err = arch_timer_available();
>   	if (err)
>   		return err;
>
> -	setup_sched_clock(arch_counter_get_cntvct32, 32, arch_timer_rate);
> +	if (arch_timer_use_virtual)
> +		cnt32 = arch_counter_get_cntvct32;
> +	else
> +		cnt32 = arch_counter_get_cntpct32;
> +
> +	setup_sched_clock(cnt32, 32, arch_timer_rate);
>   	return 0;
>   }
>

-- 
Thanks
- Cyril