Unify arm_generic and arch_timer drivers

---
 arch/arm/kernel/arch_timer.c | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/kernel/arch_timer.c +2 -0

index c8ef207..6dd73c6 100644
@@ -488,6 +488,8 @@ int __init arch_timer_of_register(void)
 	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
 		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
 
+	of_node_put(np);
+
 	/*
 	 * If no interrupt provided for virtual timer, we'll have to
 	 * stick to the physical timer. It'd better be accessible...

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:33 UTC #2 lore raw

PATCH v5 1/16 arm: arch_timer: balance device_node refcounting

Quoted 10 lines · Mark Rutland

When we get the device_node for the arch timer, it's refcount is automatically incremented in of_find_matching_node, but it is never decremented.
This patch decrements the refcount on the node after we're finished using it.
Reported-by Will Deacon <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Acked-by Santosh Shilimkar <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #3 lore raw

PATCH v5 2/16 arm: arch_timer: remove redundant available check

This check is a holdover from the pre-devicetree days. As the timer is not probed except by platforms which register it via devicetree, it's not strictly necessary.

---
 arch/arm/kernel/arch_timer.c | 12 ------------
 1 file changed, 12 deletions(-)

arch/arm/kernel/arch_timer.c +0 ~~-12~~

index 6dd73c6..1bb3b582 100644
@@ -20,11 +20,9 @@
 #include <linux/of_irq.h>
 #include <linux/io.h>
 
-#include <asm/cputype.h>
 #include <asm/delay.h>
 #include <asm/localtimer.h>
 #include <asm/arch_timer.h>
-#include <asm/system_info.h>
 #include <asm/sched_clock.h>
 
 static unsigned long arch_timer_rate;

@@ -259,20 +257,10 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 	return 0;
 }
 
-/* Is the optional system timer available? */
-static int local_timer_is_architected(void)
-{
-	return (cpu_architecture() >= CPU_ARCH_ARMv7) &&
-	       ((read_cpuid_ext(CPUID_EXT_PFR1) >> 16) & 0xf) == 1;
-}
-
 static int arch_timer_available(void)
 {
 	unsigned long freq;
 
-	if (!local_timer_is_architected())
-		return -ENXIO;
-
 	if (arch_timer_rate == 0) {
 		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
 					   ARCH_TIMER_REG_FREQ);

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #4 lore raw

PATCH v5 3/16 arm: arch_timer: use u64/u32 for register data

To ensure the correct size of types, use u64 for the return value of arch_timer_get_cnt{p,v}ct, and u32 for arch_timer_rate, matching the size of the registers these values are taken from. While we're changing them anyway, simplify the implementation of arch_timer_get_cnt{p,v}ct.

---
 arch/arm/kernel/arch_timer.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

arch/arm/kernel/arch_timer.c +11 ~~-19~~

index 1bb3b582..498c29f 100644
@@ -25,7 +25,7 @@
 #include <asm/arch_timer.h>
 #include <asm/sched_clock.h>
 
-static unsigned long arch_timer_rate;
+static u32 arch_timer_rate;
 
 enum ppi_nr {
 	PHYS_SECURE_PPI,

@@ -121,27 +121,18 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
 	return val;
 }
 
-static inline cycle_t arch_timer_counter_read(const int access)
+static inline u64 arch_counter_get_cntpct(void)
 {
-	cycle_t cval = 0;
-
-	if (access == ARCH_TIMER_PHYS_ACCESS)
-		asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
-
-	if (access == ARCH_TIMER_VIRT_ACCESS)
-		asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
-
+	u64 cval;
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
 	return cval;
 }
 
-static inline cycle_t arch_counter_get_cntpct(void)
-{
-	return arch_timer_counter_read(ARCH_TIMER_PHYS_ACCESS);
-}
-
-static inline cycle_t arch_counter_get_cntvct(void)
+static inline u64 arch_counter_get_cntvct(void)
 {
-	return arch_timer_counter_read(ARCH_TIMER_VIRT_ACCESS);
+	u64 cval;
+	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
 }
 
 static irqreturn_t inline timer_handler(const int access,

@@ -259,7 +250,7 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 
 static int arch_timer_available(void)
 {
-	unsigned long freq;
+	u32 freq;
 
 	if (arch_timer_rate == 0) {
 		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,

@@ -275,7 +266,8 @@ static int arch_timer_available(void)
 	}
 
 	pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
-		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100,
+		     (unsigned long)arch_timer_rate / 1000000,
+		     (unsigned long)(arch_timer_rate / 10000) % 100,
 		     arch_timer_use_virtual ? "virt" : "phys");
 	return 0;
 }

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #5 lore raw

PATCH v5 4/16 arm: arch_timer: standardise counter reading

We're currently inconsistent with respect to our accesses to the physical and virtual counters, mixing and matching the two.

This patch introduces and uses a function pointer for accessing the correct counter based on whether we're using physical or virtual interrupts. All current accesses to the counter accessors are redirected through it.

When the driver is moved out to drivers/clocksource, there's the possibility that code called before the timer code is initialised will attempt to call arch_timer_read_counter (e.g. sched_clock for AArch64). To avoid having to have to check whether the timer has been initialised either in arch_timer_read_counter or one of it's callers, a default implementation is assigned that simply returns 0.

Cc Catalin Marinas <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
 arch/arm/kernel/arch_timer.c | 58 +++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 36 deletions(-)

arch/arm/kernel/arch_timer.c +22 ~~-36~~

index 498c29f..eb78f43 100644
@@ -272,51 +272,37 @@ static int arch_timer_available(void)
 	return 0;
 }
 
-static u32 notrace arch_counter_get_cntpct32(void)
+/*
+ * Some external users of arch_timer_read_counter (e.g. sched_clock) may try to
+ * call it before it has been initialised. Rather than incur a performance
+ * penalty checking for initialisation, provide a default implementation that
+ * won't lead to time appearing to jump backwards.
+ */
+static u64 arch_timer_read_zero(void)
 {
-	cycle_t cnt = arch_counter_get_cntpct();
-
-	/*
-	 * 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;
+	return 0;
 }
 
-static u32 notrace arch_counter_get_cntvct32(void)
-{
-	cycle_t cnt = arch_counter_get_cntvct();
+u64 (*arch_timer_read_counter)(void) = arch_timer_read_zero;
 
-	/*
-	 * 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;
+static u32 arch_timer_read_counter32(void)
+{
+	return arch_timer_read_counter();
 }
 
 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();
+	return arch_timer_read_counter();
 }
 
 static unsigned long arch_timer_read_current_timer(void)
 {
-	return arch_counter_get_cntpct();
+	return arch_timer_read_counter();
 }
 
 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();
+	return arch_timer_read_counter();
 }
 
 static struct clocksource clocksource_counter = {

@@ -484,23 +470,23 @@ int __init arch_timer_of_register(void)
 		}
 	}
 
+	if (arch_timer_use_virtual)
+		arch_timer_read_counter = arch_counter_get_cntvct;
+	else
+		arch_timer_read_counter = arch_counter_get_cntpct;
+
 	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;
 
-	if (arch_timer_use_virtual)
-		cnt32 = arch_counter_get_cntvct32;
-	else
-		cnt32 = arch_counter_get_cntpct32;
-
-	setup_sched_clock(cnt32, 32, arch_timer_rate);
+	setup_sched_clock(arch_timer_read_counter32,
+			  32, arch_timer_rate);
 	return 0;
 }

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:34 UTC #6 lore raw

PATCH v5 4/16 arm: arch_timer: standardise counter reading

Quoted 15 lines · Mark Rutland

We're currently inconsistent with respect to our accesses to the physical and virtual counters, mixing and matching the two.
This patch introduces and uses a function pointer for accessing the correct counter based on whether we're using physical or virtual interrupts. All current accesses to the counter accessors are redirected through it.
When the driver is moved out to drivers/clocksource, there's the possibility that code called before the timer code is initialised will attempt to call arch_timer_read_counter (e.g. sched_clock for AArch64). To avoid having to have to check whether the timer has been initialised either in arch_timer_read_counter or one of it's callers, a default implementation is assigned that simply returns 0.

Quoted 4 lines

Signed-off-by Mark Rutland <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Santosh Shilimkar <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #7 lore raw

PATCH v5 5/16 arm: arch_timer: split cntfrq accessor

The CNTFRQ register is not duplicated for physical and virtual timers, and accessing it as if it were is confusing.

Instead, use a separate accessor which doesn't take the access type as a parameter.

---
 arch/arm/kernel/arch_timer.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

arch/arm/kernel/arch_timer.c +9 -7

index eb78f43..f31c9ee 100644
@@ -51,8 +51,7 @@ static bool arch_timer_use_virtual = true;
 #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
 
 #define ARCH_TIMER_PHYS_ACCESS		0
 #define ARCH_TIMER_VIRT_ACCESS		1

@@ -101,9 +100,6 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
 		case ARCH_TIMER_REG_TVAL:
 			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
 			break;
-		case ARCH_TIMER_REG_FREQ:
-			asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
-			break;
 		}
 	}

@@ -121,6 +117,13 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
 	return val;
 }
 
+static inline u32 arch_timer_get_cntfrq(void)
+{
+	u32 val;
+	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
+	return val;
+}
+
 static inline u64 arch_counter_get_cntpct(void)
 {
 	u64 cval;

@@ -253,8 +256,7 @@ static int arch_timer_available(void)
 	u32 freq;
 
 	if (arch_timer_rate == 0) {
-		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
-					   ARCH_TIMER_REG_FREQ);
+		freq = arch_timer_get_cntfrq();
 
 		/* Check the timer frequency. */
 		if (freq == 0) {

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #8 lore raw

PATCH v5 6/16 arm: arch_timer: factor out register accessors

Currently the arch_timer register accessors are thrown together with the main driver, preventing us from porting the driver to other architectures.

This patch moves the register accessors into a header file, as with the arm64 version. Constants required by the accessors are also moved.

---
 arch/arm/include/asm/arch_timer.h | 94 +++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/arch_timer.c      | 92 --------------------------------------
 2 files changed, 94 insertions(+), 92 deletions(-)

arch/arm/include/asm/arch_timer.h +94 -0

index d40229d..db0fdc4 100644
@@ -1,13 +1,107 @@
 #ifndef __ASMARM_ARCH_TIMER_H
 #define __ASMARM_ARCH_TIMER_H
 
+#include <asm/barrier.h>
 #include <asm/errno.h>
 #include <linux/clocksource.h>
+#include <linux/types.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);
+
+#define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
+#define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
+#define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
+
+#define ARCH_TIMER_REG_CTRL		0
+#define ARCH_TIMER_REG_TVAL		1
+
+#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)
+{
+	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;
+		}
+	}
+}
+
+static inline u32 arch_timer_reg_read(const int access, const int reg)
+{
+	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;
+		}
+	}
+
+	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 inline u32 arch_timer_get_cntfrq(void)
+{
+	u32 val;
+	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
+	return val;
+}
+
+static inline u64 arch_counter_get_cntpct(void)
+{
+	u64 cval;
+
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
+
+static inline u64 arch_counter_get_cntvct(void)
+{
+	u64 cval;
+
+	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
 #else
 static inline int arch_timer_of_register(void)
 {

arch/arm/kernel/arch_timer.c +0 ~~-92~~

98 lines not on this page · raw message

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #9 lore raw

PATCH v5 7/16 arm: arch_timer: add isbs to register accessors

Without the isbs in arch_timer_get_cnt{p,v}ct the cpu may speculate reads and return stale values. This could be bad for code sensitive to changes in expected deltas between calls (e.g. the delay loop).

Without isbs in arch_timer_reg_write the processor may reorder instructions around enabling/disabling of the timer or writing the compare value, which we probably don't want.

This patch adds isbs to prevent those issues.

---
 arch/arm/include/asm/arch_timer.h | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm/include/asm/arch_timer.h +4 -0

index db0fdc4..75975d9 100644
@@ -49,6 +49,8 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val
 			break;
 		}
 	}
+
+	isb();
 }
 
 static inline u32 arch_timer_reg_read(const int access, const int reg)

@@ -91,6 +93,7 @@ static inline u64 arch_counter_get_cntpct(void)
 {
 	u64 cval;
 
+	isb();
 	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
 	return cval;
 }

@@ -99,6 +102,7 @@ static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;
 
+	isb();
 	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
 	return cval;
 }

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:35 UTC #10 lore raw

PATCH v5 7/16 arm: arch_timer: add isbs to register accessors

Quoted 11 lines · Mark Rutland

Without the isbs in arch_timer_get_cnt{p,v}ct the cpu may speculate reads and return stale values. This could be bad for code sensitive to changes in expected deltas between calls (e.g. the delay loop).
Without isbs in arch_timer_reg_write the processor may reorder instructions around enabling/disabling of the timer or writing the compare value, which we probably don't want.
This patch adds isbs to prevent those issues.
Signed-off-by Mark Rutland <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #11 lore raw

PATCH v5 8/16 arm: arch_timer: divorce from local_timer api

Currently, the arch_timer driver is tied to the arm port, as it relies on code in arch/arm/smp.c to setup and teardown timers as cores are hotplugged on and off. The timer is registered through an arm-specific registration mechanism, preventing sharing the driver with the arm64 port.

This patch moves the driver to using a cpu notifier instead, making it easier to port.

---
 arch/arm/kernel/arch_timer.c | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

arch/arm/kernel/arch_timer.c +29 ~~-23~~

index e973cc0..c8dfec0 100644
@@ -21,7 +21,6 @@
 #include <linux/io.h>
 
 #include <asm/delay.h>
-#include <asm/localtimer.h>
 #include <asm/arch_timer.h>
 #include <asm/sched_clock.h>

@@ -37,7 +36,7 @@ enum ppi_nr {
 
 static int arch_timer_ppi[MAX_TIMER_PPI];
 
-static struct clock_event_device __percpu **arch_timer_evt;
+static struct clock_event_device __percpu *arch_timer_evt;
 static struct delay_timer arch_delay_timer;
 
 static bool arch_timer_use_virtual = true;

@@ -63,14 +62,14 @@ static irqreturn_t inline timer_handler(const int access,
 
 static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
+	struct clock_event_device *evt = dev_id;
 
 	return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt);
 }
 
 static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
+	struct clock_event_device *evt = dev_id;
 
 	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
 }

@@ -141,13 +140,13 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 		clk->set_next_event = arch_timer_set_next_event_phys;
 	}
 
+	clk->cpumask = cpumask_of(smp_processor_id());
+
 	clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, NULL);
 
 	clockevents_config_and_register(clk, arch_timer_rate,
 					0xf, 0x7fffffff);
 
-	*__this_cpu_ptr(arch_timer_evt) = clk;
-
 	if (arch_timer_use_virtual)
 		enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
 	else {

@@ -251,12 +250,26 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
-static struct local_timer_ops arch_timer_ops __cpuinitdata = {
-	.setup	= arch_timer_setup,
-	.stop	= arch_timer_stop,
-};
+static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self,
+					   unsigned long action, void *hcpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt);
+
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		arch_timer_setup(evt);
+		break;
+	case CPU_DYING:
+		arch_timer_stop(evt);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
 
-static struct clock_event_device arch_timer_global_evt;
+static struct notifier_block arch_timer_cpu_nb __cpuinitdata = {
+	.notifier_call = arch_timer_cpu_notify,
+};
 
 static int __init arch_timer_register(void)
 {

@@ -267,7 +280,7 @@ static int __init arch_timer_register(void)
 	if (err)
 		goto out;
 
-	arch_timer_evt = alloc_percpu(struct clock_event_device *);
+	arch_timer_evt = alloc_percpu(struct clock_event_device);
 	if (!arch_timer_evt) {
 		err = -ENOMEM;
 		goto out;

@@ -303,20 +316,13 @@ static int __init arch_timer_register(void)
 		goto out_free;
 	}
 
-	err = local_timer_register(&arch_timer_ops);
-	if (err) {
-		/*
-		 * We couldn't register as a local timer (could be
-		 * because we're on a UP platform, or because some
-		 * other local timer is already present...). Try as a
-		 * global timer instead.
-		 */
-		arch_timer_global_evt.cpumask = cpumask_of(0);
-		err = arch_timer_setup(&arch_timer_global_evt);
-	}
+	err = register_cpu_notifier(&arch_timer_cpu_nb);
 	if (err)
 		goto out_free_irq;
 
+	/* Immediately configure the timer on the boot CPU */
+	arch_timer_setup(this_cpu_ptr(arch_timer_evt));
+
 	/* Use the architected timer for the delay loop. */
 	arch_delay_timer.read_current_timer = &arch_timer_read_current_timer;
 	arch_delay_timer.freq = arch_timer_rate;

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #12 lore raw

PATCH v5 9/16 arm: arch_timer: add arch_counter_set_user_access

Several bits in CNTKCTL reset to 0, including PL0VTEN. For architectures using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. For architectures without a vDSO, it's best to leave the bits set to 0 for now to ensure that if and when support is added, it's implemented sanely architecture wide.

As the bootloader might set PL0VTEN to a value that doesn't correspond to that which the kernel prefers, we must explicitly set it to the architecture port's preferred value.

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the architecture. For arch/arm, this currently means disabling all userspace access.

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

arch/arm/include/asm/arch_timer.h +12 -0

index 75975d9..729f6d9 100644
@@ -106,6 +106,18 @@ static inline u64 arch_counter_get_cntvct(void)
 	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
 	return cval;
 }
+
+static inline void __cpuinit arch_counter_set_user_access(void)
+{
+	u32 cntkctl;
+
+	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
+
+	/* disable user access to everything */
+	cntkctl &= ~((3 << 8) | (7 << 0));
+
+	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+}
 #else
 static inline int arch_timer_of_register(void)
 {

arch/arm/kernel/arch_timer.c +2 -0

index c8dfec0..94f5033 100644
@@ -155,6 +155,8 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 			enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
 	}
 
+	arch_counter_set_user_access();
+
 	return 0;
 }

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:36 UTC #13 lore raw

PATCH v5 9/16 arm: arch_timer: add arch_counter_set_user_access

Quoted 11 lines · Mark Rutland

Several bits in CNTKCTL reset to 0, including PL0VTEN. For architectures using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. For architectures without a vDSO, it's best to leave the bits set to 0 for now to ensure that if and when support is added, it's implemented sanely architecture wide.
As the bootloader might set PL0VTEN to a value that doesn't correspond to that which the kernel prefers, we must explicitly set it to the architecture port's preferred value.

Quoted 5 lines

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the architecture. For arch/arm, this currently means disabling all userspace access.
Signed-off-by Mark Rutland <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #14 lore raw

PATCH v5 10/16 arm: arch_timer: move core to drivers/clocksource

The core functionality of the arch_timer driver is not directly tied to anything under arch/arm, and can be split out.

This patch factors out the core of the arch_timer driver, so it can be shared with other architectures. A couple of functions are added so that architecture-specific code can interact with the driver without needing to touch its internals.

The ARM_ARCH_TIMER config variable is moved out to drivers/clocksource/Kconfig, existing uses in arch/arm are replaced with HAVE_ARM_ARCH_TIMER, which selects it.

---
 arch/arm/Kconfig                     |   3 +-
 arch/arm/include/asm/arch_timer.h    |  19 +-
 arch/arm/kernel/arch_timer.c         | 386 ++---------------------------------
 arch/arm/mach-omap2/Kconfig          |   2 +-
 drivers/clocksource/Kconfig          |   3 +
 drivers/clocksource/Makefile         |   1 +
 drivers/clocksource/arm_arch_timer.c | 385 ++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h |  63 ++++++
 8 files changed, 476 insertions(+), 386 deletions(-)
 create mode 100644 drivers/clocksource/arm_arch_timer.c
 create mode 100644 include/clocksource/arm_arch_timer.h

arch/arm/Kconfig +2 -1

index 67874b8..e1162f5 100644
@@ -1572,9 +1572,10 @@ config HAVE_ARM_SCU
 	help
 	  This option enables support for the ARM system coherency unit
 
-config ARM_ARCH_TIMER
+config HAVE_ARM_ARCH_TIMER
 	bool "Architected timer support"
 	depends on CPU_V7
+	select ARM_ARCH_TIMER
 	help
 	  This option enables support for the ARM architected timer

arch/arm/include/asm/arch_timer.h +3 ~~-16~~

index 729f6d9..7ade91d 100644
@@ -4,22 +4,14 @@
 #include <asm/barrier.h>
 #include <asm/errno.h>
 #include <linux/clocksource.h>
+#include <linux/init.h>
 #include <linux/types.h>
 
+#include <clocksource/arm_arch_timer.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);
-
-#define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
-#define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
-#define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
-
-#define ARCH_TIMER_REG_CTRL		0
-#define ARCH_TIMER_REG_TVAL		1
-
-#define ARCH_TIMER_PHYS_ACCESS		0
-#define ARCH_TIMER_VIRT_ACCESS		1
 
 /*
  * These register accessors are marked inline so the compiler can

@@ -128,11 +120,6 @@ static inline int arch_timer_sched_clock_init(void)
 {
 	return -ENXIO;
 }
-
-static inline struct timecounter *arch_timer_get_timecounter(void)
-{
-	return NULL;
-}
 #endif
 
 #endif

arch/arm/kernel/arch_timer.c +18 ~~-368~~

420 lines not on this page · raw message

arch/arm/mach-omap2/Kconfig +1 -1

index 41b581f..9d7909e 100644
@@ -76,12 +76,12 @@ config ARCH_OMAP4
 
 config SOC_OMAP5
 	bool "TI OMAP5"
-	select ARM_ARCH_TIMER
 	select ARM_CPU_SUSPEND if PM
 	select ARM_GIC
 	select CPU_V7
 	select HAVE_SMP
 	select COMMON_CLK
+	select HAVE_ARM_ARCH_TIMER
 
 comment "OMAP Core Type"
 	depends on ARCH_OMAP2

drivers/clocksource/Kconfig +3 -0

index 7fdcbd3..dbb085a 100644
@@ -58,3 +58,6 @@ config CLKSRC_ARM_GENERIC
 	def_bool y if ARM64
 	help
 	  This option enables support for the ARM generic timer.
+
+config ARM_ARCH_TIMER
+	bool

drivers/clocksource/Makefile +1 -0

index f93453d..32f858c 100644
@@ -18,3 +18,4 @@ obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_SUNXI_TIMER)	+= sunxi_timer.o
 
 obj-$(CONFIG_CLKSRC_ARM_GENERIC)	+= arm_generic.o
+obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o

drivers/clocksource/arm_arch_timer.c new file +385 -0

385 lines not on this page · raw message

include/clocksource/arm_arch_timer.h new file +63 -0

63 lines not on this page · raw message

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #15 lore raw

PATCH v5 11/16 arm64: arm_generic: prevent reading stale time

Currently arch_counter_get_cnt{p,v}ct can be speculated, allowing for stale time values to be read. This could be problematic for the delay loop and other sensitive functions, as the time delta could jump around unexpectedly.

This patch adds isbs to arch_counter_get_cnt{p,v}ct, preventing this possibility.

---
 arch/arm64/include/asm/arm_generic.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/arm_generic.h +2 -0

index df2aeb8..6ece2f1 100644
@@ -83,6 +83,7 @@ static inline cycle_t arch_counter_get_cntpct(void)
 {
 	cycle_t cval;
 
+	isb();
 	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
 
 	return cval;

@@ -92,6 +93,7 @@ static inline cycle_t arch_counter_get_cntvct(void)
 {
 	cycle_t cval;
 
+	isb();
 	asm volatile("mrs %0, cntvct_el0" : "=r" (cval));
 
 	return cval;

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:36 UTC #16 lore raw

PATCH v5 11/16 arm64: arm_generic: prevent reading stale time

Quoted 9 lines · Mark Rutland

Currently arch_counter_get_cnt{p,v}ct can be speculated, allowing for stale time values to be read. This could be problematic for the delay loop and other sensitive functions, as the time delta could jump around unexpectedly.
This patch adds isbs to arch_counter_get_cnt{p,v}ct, preventing this possibility.
Signed-off-by Mark Rutland <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #17 lore raw

PATCH v5 12/16 arm64: move from arm_generic to arm_arch_timer

The arch_timer driver supports a superset of the functionality of the arm_generic driver, and is not tied to a particular arch.

This patch moves arm64 to use the arch_timer driver, gaining additional functionality in doing so, and removes the (now unused) arm_generic driver. Timer-related hooks specific to arm64 are moved into arch/arm64/kernel/time.c.

---
 arch/arm64/Kconfig                   |   1 +
 arch/arm64/include/asm/arch_timer.h  | 133 ++++++++++++++++++++
 arch/arm64/include/asm/arm_generic.h | 102 ---------------
 arch/arm64/kernel/time.c             |  29 ++++-
 drivers/clocksource/Kconfig          |   5 -
 drivers/clocksource/Makefile         |   1 -
 drivers/clocksource/arm_arch_timer.c |   1 +
 drivers/clocksource/arm_generic.c    | 232 -----------------------------------
 include/clocksource/arm_generic.h    |  21 ----
 9 files changed, 162 insertions(+), 363 deletions(-)
 create mode 100644 arch/arm64/include/asm/arch_timer.h
 delete mode 100644 arch/arm64/include/asm/arm_generic.h
 delete mode 100644 drivers/clocksource/arm_generic.c
 delete mode 100644 include/clocksource/arm_generic.h

arch/arm64/Kconfig +1 -0

index f8f362a..2b6cef6 100644
@@ -3,6 +3,7 @@ config ARM64
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
 	select ARM_AMBA
+	select ARM_ARCH_TIMER
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select GENERIC_CLOCKEVENTS

arch/arm64/include/asm/arch_timer.h new file +133 -0

133 lines not on this page · raw message

arch/arm64/include/asm/arm_generic.h deleted +0 ~~-102~~

102 lines not on this page · raw message

arch/arm64/kernel/time.c +27 -2

index 3b4b725..b0ef18d 100644
@@ -31,8 +31,9 @@
 #include <linux/syscore_ops.h>
 #include <linux/timer.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
-#include <clocksource/arm_generic.h>
+#include <clocksource/arm_arch_timer.h>
 
 #include <asm/thread_info.h>
 #include <asm/stacktrace.h>

@@ -59,7 +60,31 @@ unsigned long profile_pc(struct pt_regs *regs)
 EXPORT_SYMBOL(profile_pc);
 #endif
 
+static u64 sched_clock_mult __read_mostly;
+
+unsigned long long notrace sched_clock(void)
+{
+	return arch_timer_read_counter() * sched_clock_mult;
+}
+
+int read_current_timer(unsigned long *timer_value)
+{
+	*timer_value = arch_timer_read_counter();
+	return 0;
+}
+
 void __init time_init(void)
 {
-	arm_generic_timer_init();
+	u32 arch_timer_rate;
+
+	if (arch_timer_init())
+		panic("Unable to initialise architected timer.\n");
+
+	arch_timer_rate = arch_timer_get_rate();
+
+	/* Cache the sched_clock multiplier to save a divide in the hot path. */
+	sched_clock_mult = NSEC_PER_SEC / arch_timer_rate;
+
+	/* Calibrate the delay loop directly */
+	lpj_fine = arch_timer_rate / HZ;
 }

drivers/clocksource/Kconfig +0 -5

index dbb085a..6479842 100644
@@ -54,10 +54,5 @@ config CLKSRC_DBX500_PRCMU_SCHED_CLOCK
 	help
 	  Use the always on PRCMU Timer as sched_clock
 
-config CLKSRC_ARM_GENERIC
-	def_bool y if ARM64
-	help
-	  This option enables support for the ARM generic timer.
-
 config ARM_ARCH_TIMER
 	bool

drivers/clocksource/Makefile +0 -1

index 32f858c..e69511c 100644
@@ -17,5 +17,4 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_SUNXI_TIMER)	+= sunxi_timer.o
 
-obj-$(CONFIG_CLKSRC_ARM_GENERIC)	+= arm_generic.o
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o

drivers/clocksource/arm_arch_timer.c +1 -0

index 3e4739d..ec30a73 100644
@@ -338,6 +338,7 @@ out:
 
 static const struct of_device_id arch_timer_of_match[] __initconst = {
 	{ .compatible	= "arm,armv7-timer",	},
+	{ .compatible	= "arm,armv8-timer",	},
 	{},
 };

drivers/clocksource/arm_generic.c deleted +0 ~~-232~~

232 lines not on this page · raw message

include/clocksource/arm_generic.h deleted +0 ~~-21~~

deleted file mode 100644
index 5b41b0d..0000000
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2012 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef __CLKSOURCE_ARM_GENERIC_H
-#define __CLKSOURCE_ARM_GENERIC_H
-
-extern int arm_generic_timer_init(void);
-
-#endif

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #18 lore raw

PATCH v5 13/16 Documentation: Add ARMv8 to arch_timer devicetree

Currently the documentation for the arch_timer devicetree binding only lists "arm,armv7-timer".

Add "arm,armv8-timer" to the list of compatible strings.

---
 Documentation/devicetree/bindings/arm/arch_timer.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/arm/arch_timer.txt +4 -3

index 52478c8..20746e5 100644
@@ -1,13 +1,14 @@
 * ARM architected timer
 
-ARM Cortex-A7 and Cortex-A15 have a per-core architected timer, which
-provides per-cpu timers.
+ARM cores may have a per-core architected timer, which provides per-cpu timers.
 
 The timer is attached to a GIC to deliver its per-processor interrupts.
 
 ** Timer node properties:
 
-- compatible : Should at least contain "arm,armv7-timer".
+- compatible : Should at least contain one of
+	"arm,armv7-timer"
+	"arm,armv8-timer"
 
 - interrupts : Interrupt list for secure, non-secure, virtual and
   hypervisor timers, in that order.

Signature · 1 line

1.8.1.1

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #19 lore raw

PATCH v5 14/16 ARM: arch_timers: switch to physical timers if HYP mode is available

From: Marc Zyngier <[email protected]>

If we're booted in HYP mode, it is possible that we'll run some kind of virtualized environment. In this case, it is a better to switch to the physical timers, and leave the virtual timers to guests.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/clocksource/arm_arch_timer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/clocksource/arm_arch_timer.c +6 -1

index ec30a73..d7ad425 100644
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 
 #include <asm/arch_timer.h>
+#include <asm/virt.h>
 
 #include <clocksource/arm_arch_timer.h>

@@ -364,10 +365,14 @@ int __init arch_timer_init(void)
 	of_node_put(np);
 
 	/*
+	 * If HYP mode is available, we know that the physical timer
+	 * has been configured to be accessible from PL1. Use it, so
+	 * that a guest can use the virtual timer instead.
+	 *
 	 * 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]) {
+	if (is_hyp_mode_available() || !arch_timer_ppi[VIRT_PPI]) {
 		arch_timer_use_virtual = false;
 
 		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:37 UTC #20 lore raw

PATCH v5 14/16 ARM: arch_timers: switch to physical timers if HYP mode is available

Quoted 9 lines · Mark Rutland

From: Marc Zyngier <[email protected]>
If we're booted in HYP mode, it is possible that we'll run some kind of virtualized environment. In this case, it is a better to switch to the physical timers, and leave the virtual timers to guests.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #21 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

From: Marc Zyngier <[email protected]>

In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.

Signed-off-by Marc Zyngier <[email protected]>

Cc Dave Martin <[email protected]>

---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kernel/hyp-stub.S +3 -0

index 65b2417..455603a 100644
@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
 1:
 #endif

Signature · 1 line

1.8.1.1

Dave Martin

2013-02-01 11:13 UTC #22 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

Quoted 17 lines · Mark Rutland

From: Marc Zyngier <[email protected]>
In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Cc Dave Martin <[email protected]>
---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/kernel/hyp-stub.S +0 -0
index 65b2417..455603a 100644

arch/arm/kernel/hyp-stub.S THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE · line 157

Quoted 6 lines

arch/arm/kernel/hyp-stub.S +3 -0

@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF

Is this required for safety, or is it more a sanity feature?

The architected timer counters are supposed to be monotonic time sources only, so applying a random offset shouldn't really change anything.

The main thing I can think of is that it is easier for the host to manage guests' virtual counter offsets if the host's offset is 0 (and we don't really want to be changing the host offset after the host kernel boots).

Cheers
---Dave

Marc Zyngier

2013-02-01 11:43 UTC #23 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

Hi Dave,

Quoted 27 lines · Dave Martin

Quoted 17 lines · Mark Rutland
From: Marc Zyngier <[email protected]>
In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Cc Dave Martin <[email protected]>
---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)
arch/arm/kernel/hyp-stub.S +0 -0
index 65b2417..455603a 100644
arch/arm/kernel/hyp-stub.S THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE · line 157
Quoted 6 lines
arch/arm/kernel/hyp-stub.S +3 -0
@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
Is this required for safety, or is it more a sanity feature?

Quoted 7 lines

The architected timer counters are supposed to be monotonic time sources only, so applying a random offset shouldn't really change anything.
The main thing I can think of is that it is easier for the host to manage guests' virtual counter offsets if the host's offset is 0 (and we don't really want to be changing the host offset after the host kernel boots).

As you noticed, the offset itself doesn't matter as long as it is constant. However, setting it to zero makes it nicer to the hypervisor:
no need to remember the host offset, just reset it to zero when exiting a guest.

M.

Signature · 1 line

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

Mark Rutland

author maintainer

2013-02-01 11:46 UTC #24 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

In reply to Dave Martin

Quoted 13 lines · Dave Martin

Quoted 12 lines · Mark Rutland
From: Marc Zyngier <[email protected]>
In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Cc Dave Martin <[email protected]>
---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)

Quoted 13 lines

arch/arm/kernel/hyp-stub.S THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE · line 157
Quoted 11 lines
arch/arm/kernel/hyp-stub.S +3 -0
index 65b2417..455603a 100644
@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
Is this required for safety, or is it more a sanity feature?

This makes more sense with the next patch, which makes the arch_timer driver always use the virtual counters (to avoid indirection in the fast path and messy races with the setup of function pointers otherwise).

It's required for safety when hyp mode is enabled, and the arch_timer driver uses the physical timers in combination with the virtual counters. Either the driver has to apply CNTVOFF manually when setting the physical timers, or the physical timers and virtual counters need the same view of time (i.e. CNTVOFF == 0).

It also brings us in line with arm64, which always uses the virtual counter for its vDSO.

Quoted 3 lines

The architected timer counters are supposed to be monotonic time sources only, so applying a random offset shouldn't really change anything.

This is true except when we want to use the physical timers as described above.

Quoted 5 lines

The main thing I can think of is that it is easier for the host to manage guests' virtual counter offsets if the host's offset is 0 (and we don't really want to be changing the host offset after the host kernel boots).

That's pretty much it. We don't want to have to further separate the handling of the timer for host and guest. By having CNTVOFF as zero for the host, we don't need to duplicate reading of the timers and/or incur an additional overhead on reading them.

Thanks,
Mark.

Dave Martin

2013-02-01 18:07 UTC #25 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

Quoted 32 lines · Mark Rutland

Quoted 13 lines · Dave Martin
Quoted 12 lines · Mark Rutland
From: Marc Zyngier <[email protected]>
In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Cc Dave Martin <[email protected]>
---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)
Quoted 13 lines
arch/arm/kernel/hyp-stub.S THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE · line 157
Quoted 11 lines
arch/arm/kernel/hyp-stub.S +3 -0
index 65b2417..455603a 100644
@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
Is this required for safety, or is it more a sanity feature?
This makes more sense with the next patch, which makes the arch_timer driver always use the virtual counters (to avoid indirection in the fast path and messy races with the setup of function pointers otherwise).

Quoted 8 lines

It's required for safety when hyp mode is enabled, and the arch_timer driver uses the physical timers in combination with the virtual counters. Either the driver has to apply CNTVOFF manually when setting the physical timers, or the physical timers and virtual counters need the same view of time (i.e. CNTVOFF == 0).
It also brings us in line with arm64, which always uses the virtual counter for its vDSO.

OK. This definitely sounds like the correct model.

Quoted 13 lines

Quoted 4 lines
The architected timer counters are supposed to be monotonic time sources only, so applying a random offset shouldn't really change anything.
This is true except when we want to use the physical timers as described above.
Quoted 5 lines
The main thing I can think of is that it is easier for the host to manage guests' virtual counter offsets if the host's offset is 0 (and we don't really want to be changing the host offset after the host kernel boots).

Quoted 4 lines

That's pretty much it. We don't want to have to further separate the handling of the timer for host and guest. By having CNTVOFF as zero for the host, we don't need to duplicate reading of the timers and/or incur an additional overhead on reading them.

That all sounds sensible. FWIW:

Reviewed-by Dave Martin <[email protected]>

Cheers
---Dave

Mark Rutland

author maintainer

2013-02-04 09:29 UTC #26 lore raw

PATCH v5 15/16 ARM: hyp: initialize CNTVOFF to zero

Quoted 39 lines · Dave Martin

Quoted 38 lines · Mark Rutland
Quoted 13 lines · Dave Martin
Quoted 12 lines · Mark Rutland
From: Marc Zyngier <[email protected]>
In order to be able to use the virtual counter in a safe way, make sure it is initialized to zero before dropping to SVC.
Signed-off-by Marc Zyngier <[email protected]>
Signed-off-by Mark Rutland <[email protected]>
Cc Dave Martin <[email protected]>
---
 arch/arm/kernel/hyp-stub.S | 3 +++
 1 file changed, 3 insertions(+)
Quoted 13 lines
arch/arm/kernel/hyp-stub.S THUMB( orr r7, #(1 << 30) ) @ HSCTLR.TE · line 157
Quoted 11 lines
arch/arm/kernel/hyp-stub.S +3 -0
index 65b2417..455603a 100644
@@ -152,6 +152,9 @@ THUMB(	orr	r7, #(1 << 30)	)	@ HSCTLR.TE
 	mrc	p15, 4, r7, c14, c1, 0	@ CNTHCTL
 	orr	r7, r7, #3		@ PL1PCEN | PL1PCTEN
 	mcr	p15, 4, r7, c14, c1, 0	@ CNTHCTL
+	mov	r6, #0
+	mov	r7, #0
+	mcrr	p15, 4, r6, r7, c14	@ CNTVOFF
Is this required for safety, or is it more a sanity feature?
This makes more sense with the next patch, which makes the arch_timer driver always use the virtual counters (to avoid indirection in the fast path and messy races with the setup of function pointers otherwise).
It's required for safety when hyp mode is enabled, and the arch_timer driver uses the physical timers in combination with the virtual counters. Either the driver has to apply CNTVOFF manually when setting the physical timers, or the physical timers and virtual counters need the same view of time (i.e. CNTVOFF == 0).

Quoted 4 lines

Quoted 2 lines
It also brings us in line with arm64, which always uses the virtual counter for its vDSO.
OK. This definitely sounds like the correct model.

Good to hear.

Quoted 14 lines

Quoted 14 lines
Quoted 5 lines
The architected timer counters are supposed to be monotonic time sources only, so applying a random offset shouldn't really change anything.
This is true except when we want to use the physical timers as described above.
Quoted 5 lines
The main thing I can think of is that it is easier for the host to manage guests' virtual counter offsets if the host's offset is 0 (and we don't really want to be changing the host offset after the host kernel boots).

Quoted 8 lines

Quoted 4 lines
That's pretty much it. We don't want to have to further separate the handling of the timer for host and guest. By having CNTVOFF as zero for the host, we don't need to duplicate reading of the timers and/or incur an additional overhead on reading them.
That all sounds sensible. FWIW:
Reviewed-by Dave Martin <[email protected]>

Thanks!

Mark.

Mark Rutland

author maintainer

2013-01-31 12:15 UTC #27 lore raw

PATCH v5 16/16 clocksource: arch_timer: use virtual counters

Switching between reading the virtual or physical counters is problematic, as some core code wants a view of time before we're fully set up. Using a function pointer and switching the source after the first read can make time appear to go backwards, and having a check in the read function is an unfortunate block on what we want to be a fast path.

Instead, this patch makes us always use the virtual counters. If we're a guest, or don't have hyp mode, we'll use the virtual timers, and as such don't care about CNTVOFF as long as it doesn't change in such a way as to make time travel backwards. If we do have hyp mode, and might be a KVM host, we have to use the physical timers, and require CNTVOFF to be zero so as to have a consistent view of time between the physical timers and virtual counters.

Any code which may want to alter CNTVOFF (e.g. KVM) will need to ensure that it is zeroed when entering the host.

---
 drivers/clocksource/arm_arch_timer.c | 21 ++++-----------------
 include/clocksource/arm_arch_timer.h |  2 +-
 2 files changed, 5 insertions(+), 18 deletions(-)

drivers/clocksource/arm_arch_timer.c +4 ~~-17~~

index d7ad425..f0705fb 100644
@@ -186,27 +186,19 @@ u32 arch_timer_get_rate(void)
 	return arch_timer_rate;
 }
 
-/*
- * Some external users of arch_timer_read_counter (e.g. sched_clock) may try to
- * call it before it has been initialised. Rather than incur a performance
- * penalty checking for initialisation, provide a default implementation that
- * won't lead to time appearing to jump backwards.
- */
-static u64 arch_timer_read_zero(void)
+u64 arch_timer_read_counter(void)
 {
-	return 0;
+	return arch_counter_get_cntvct();
 }
 
-u64 (*arch_timer_read_counter)(void) = arch_timer_read_zero;
-
 static cycle_t arch_counter_read(struct clocksource *cs)
 {
-	return arch_timer_read_counter();
+	return arch_counter_get_cntvct();
 }
 
 static cycle_t arch_counter_read_cc(const struct cyclecounter *cc)
 {
-	return arch_timer_read_counter();
+	return arch_counter_get_cntvct();
 }
 
 static struct clocksource clocksource_counter = {

@@ -382,10 +374,5 @@ int __init arch_timer_init(void)
 		}
 	}
 
-	if (arch_timer_use_virtual)
-		arch_timer_read_counter = arch_counter_get_cntvct;
-	else
-		arch_timer_read_counter = arch_counter_get_cntpct;
-
 	return arch_timer_register();
 }

include/clocksource/arm_arch_timer.h +1 -1

index b61f996..34dccbe 100644
@@ -33,7 +33,7 @@
 
 extern int arch_timer_init(void);
 extern u32 arch_timer_get_rate(void);
-extern u64 (*arch_timer_read_counter)(void);
+extern u64 arch_timer_read_counter(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
 
 #else

Signature · 1 line

1.8.1.1

Catalin Marinas

maintainer

2013-01-31 15:38 UTC #28 lore raw

PATCH v5 16/16 clocksource: arch_timer: use virtual counters

Quoted 15 lines · Mark Rutland

Switching between reading the virtual or physical counters is problematic, as some core code wants a view of time before we're fully set up. Using a function pointer and switching the source after the first read can make time appear to go backwards, and having a check in the read function is an unfortunate block on what we want to be a fast path.
Instead, this patch makes us always use the virtual counters. If we're a guest, or don't have hyp mode, we'll use the virtual timers, and as such don't care about CNTVOFF as long as it doesn't change in such a way as to make time travel backwards. If we do have hyp mode, and might be a KVM host, we have to use the physical timers, and require CNTVOFF to be zero so as to have a consistent view of time between the physical timers and virtual counters.

Quoted 4 lines

Any code which may want to alter CNTVOFF (e.g. KVM) will need to ensure that it is zeroed when entering the host.
Signed-off-by Mark Rutland <[email protected]>

Stephen Warren

2013-01-31 17:39 UTC #29 lore raw

v5 cover Unify arm_generic and arch_timer drivers