[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

The 32-bit ARM VDSO needs to know whether a generic timer is present
and whether it is suitable for use by user space.  The VDSO
initialization code currently duplicates some of the logic from the
driver to make this determination, but unfortunately it is incomplete;
it will incorrectly enable the VDSO if HYP mode is available or if no
interrupt is provided for the virtual timer (see arch_timer_init).  In
these cases the driver will switch to memory-backed access while the
VDSO will attempt to access the counter using cp15 reads.

Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
init code whether the arch timer is present and usable.

Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
---
 drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
 include/clocksource/arm_arch_timer.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 0aa135ddbf80..b75215523d2f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
 	return &timecounter;
 }
 
+/* The ARM VDSO init code needs to know:
+ * - whether a cp15-based arch timer is present; and if so
+ * - whether the physical or virtual counter is being used.
+ */
+bool arch_timer_okay_for_vdso(void)
+{
+	if (!(arch_timers_present & ARCH_CP15_TIMER))
+		return false;
+
+	return arch_timer_use_virtual;
+}
+
 static void __init arch_counter_register(unsigned type)
 {
 	u64 start_count;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 9916d0e4eff5..bfc1e95280c4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -48,6 +48,7 @@ enum arch_timer_reg {
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
+extern bool arch_timer_okay_for_vdso(void);
 
 #else
 
@@ -66,6 +67,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
 	return NULL;
 }
 
+static inline bool arch_timer_okay_for_vdso(void)
+{
+	return false;
+}
+
 #endif
 
 #endif
-- 
1.9.3

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

The 32-bit ARM VDSO needs to know whether a generic timer is present
and whether it is suitable for use by user space.  The VDSO
initialization code currently duplicates some of the logic from the
driver to make this determination, but unfortunately it is incomplete;
it will incorrectly enable the VDSO if HYP mode is available or if no
interrupt is provided for the virtual timer (see arch_timer_init).  In
these cases the driver will switch to memory-backed access while the
VDSO will attempt to access the counter using cp15 reads.

Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
init code whether the arch timer is present and usable.

Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
---
 drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
 include/clocksource/arm_arch_timer.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 0aa135ddbf80..b75215523d2f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
 	return &timecounter;
 }
 
+/* The ARM VDSO init code needs to know:
+ * - whether a cp15-based arch timer is present; and if so
+ * - whether the physical or virtual counter is being used.
+ */
+bool arch_timer_okay_for_vdso(void)
+{
+	if (!(arch_timers_present & ARCH_CP15_TIMER))
+		return false;
+
+	return arch_timer_use_virtual;
+}
+
 static void __init arch_counter_register(unsigned type)
 {
 	u64 start_count;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 9916d0e4eff5..bfc1e95280c4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -48,6 +48,7 @@ enum arch_timer_reg {
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
+extern bool arch_timer_okay_for_vdso(void);
 
 #else
 
@@ -66,6 +67,11 @@ static inline struct timecounter *arch_timer_get_timecounter(void)
 	return NULL;
 }
 
+static inline bool arch_timer_okay_for_vdso(void)
+{
+	return false;
+}
+
 #endif
 
 #endif
-- 
1.9.3

[PATCH 2/2] ARM: VDSO: use arch_timer_okay_for_vdso

[Nathan Lynch]

Use the facility now provided by the arm_arch_timer driver to
determine whether there's a usable virtual counter for the VDSO.

Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
---
 arch/arm/kernel/vdso.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..f06fd6f3f65f 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -21,7 +21,6 @@
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
-#include <linux/of.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/timekeeper_internal.h>
@@ -69,33 +68,6 @@ struct elfinfo {
  */
 static bool cntvct_ok __read_mostly;
 
-static bool __init cntvct_functional(void)
-{
-	struct device_node *np;
-	bool ret = false;
-
-	if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-		goto out;
-
-	/* The arm_arch_timer core should export
-	 * arch_timer_use_virtual or similar so we don't have to do
-	 * this.
-	 */
-	np = of_find_compatible_node(NULL, NULL, "arm,armv7-timer");
-	if (!np)
-		goto out_put;
-
-	if (of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
-		goto out_put;
-
-	ret = true;
-
-out_put:
-	of_node_put(np);
-out:
-	return ret;
-}
-
 static void * __init find_section(Elf32_Ehdr *ehdr, const char *name,
 				  unsigned long *size)
 {
@@ -208,7 +180,7 @@ static int __init vdso_init(void)
 	vdso_total_pages = 1; /* for the data/vvar page */
 	vdso_total_pages += text_pages;
 
-	cntvct_ok = cntvct_functional();
+	cntvct_ok = arch_timer_okay_for_vdso();
 
 	patch_vdso(&vdso_start);
 
-- 
1.9.3

[PATCH 2/2] ARM: VDSO: use arch_timer_okay_for_vdso

[Nathan Lynch]

Use the facility now provided by the arm_arch_timer driver to
determine whether there's a usable virtual counter for the VDSO.

Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
---
 arch/arm/kernel/vdso.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..f06fd6f3f65f 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -21,7 +21,6 @@
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
-#include <linux/of.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/timekeeper_internal.h>
@@ -69,33 +68,6 @@ struct elfinfo {
  */
 static bool cntvct_ok __read_mostly;
 
-static bool __init cntvct_functional(void)
-{
-	struct device_node *np;
-	bool ret = false;
-
-	if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-		goto out;
-
-	/* The arm_arch_timer core should export
-	 * arch_timer_use_virtual or similar so we don't have to do
-	 * this.
-	 */
-	np = of_find_compatible_node(NULL, NULL, "arm,armv7-timer");
-	if (!np)
-		goto out_put;
-
-	if (of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
-		goto out_put;
-
-	ret = true;
-
-out_put:
-	of_node_put(np);
-out:
-	return ret;
-}
-
 static void * __init find_section(Elf32_Ehdr *ehdr, const char *name,
 				  unsigned long *size)
 {
@@ -208,7 +180,7 @@ static int __init vdso_init(void)
 	vdso_total_pages = 1; /* for the data/vvar page */
 	vdso_total_pages += text_pages;
 
-	cntvct_ok = cntvct_functional();
+	cntvct_ok = arch_timer_okay_for_vdso();
 
 	patch_vdso(&vdso_start);
 
-- 
1.9.3

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Will Deacon]

On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
> The 32-bit ARM VDSO needs to know whether a generic timer is present
> and whether it is suitable for use by user space.  The VDSO
> initialization code currently duplicates some of the logic from the
> driver to make this determination, but unfortunately it is incomplete;
> it will incorrectly enable the VDSO if HYP mode is available or if no
> interrupt is provided for the virtual timer (see arch_timer_init).  In
> these cases the driver will switch to memory-backed access while the
> VDSO will attempt to access the counter using cp15 reads.
> 
> Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
> init code whether the arch timer is present and usable.
> 
> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
>  include/clocksource/arm_arch_timer.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 0aa135ddbf80..b75215523d2f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
>  	return &timecounter;
>  }
>  
> +/* The ARM VDSO init code needs to know:
> + * - whether a cp15-based arch timer is present; and if so
> + * - whether the physical or virtual counter is being used.
> + */
> +bool arch_timer_okay_for_vdso(void)
> +{
> +	if (!(arch_timers_present & ARCH_CP15_TIMER))
> +		return false;
> +
> +	return arch_timer_use_virtual;
> +}

If we're adding this, then it wouldn't hurt to use the same check in
arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
`current clocksource' knowledge in there too, to remove the hardcoded
"arch_sys_counter" check from the arch code?

Will

Re: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Will Deacon]

On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
> The 32-bit ARM VDSO needs to know whether a generic timer is present
> and whether it is suitable for use by user space.  The VDSO
> initialization code currently duplicates some of the logic from the
> driver to make this determination, but unfortunately it is incomplete;
> it will incorrectly enable the VDSO if HYP mode is available or if no
> interrupt is provided for the virtual timer (see arch_timer_init).  In
> these cases the driver will switch to memory-backed access while the
> VDSO will attempt to access the counter using cp15 reads.
> 
> Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
> init code whether the arch timer is present and usable.
> 
> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
>  include/clocksource/arm_arch_timer.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 0aa135ddbf80..b75215523d2f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
>  	return &timecounter;
>  }
>  
> +/* The ARM VDSO init code needs to know:
> + * - whether a cp15-based arch timer is present; and if so
> + * - whether the physical or virtual counter is being used.
> + */
> +bool arch_timer_okay_for_vdso(void)
> +{
> +	if (!(arch_timers_present & ARCH_CP15_TIMER))
> +		return false;
> +
> +	return arch_timer_use_virtual;
> +}

If we're adding this, then it wouldn't hurt to use the same check in
arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
`current clocksource' knowledge in there too, to remove the hardcoded
"arch_sys_counter" check from the arch code?

Will

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

On 04/27/2015 05:55 AM, Will Deacon wrote:
> On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
>> The 32-bit ARM VDSO needs to know whether a generic timer is present
>> and whether it is suitable for use by user space.  The VDSO
>> initialization code currently duplicates some of the logic from the
>> driver to make this determination, but unfortunately it is incomplete;
>> it will incorrectly enable the VDSO if HYP mode is available or if no
>> interrupt is provided for the virtual timer (see arch_timer_init).  In
>> these cases the driver will switch to memory-backed access while the
>> VDSO will attempt to access the counter using cp15 reads.
>>
>> Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
>> init code whether the arch timer is present and usable.
>>
>> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
>>  include/clocksource/arm_arch_timer.h |  6 ++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 0aa135ddbf80..b75215523d2f 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
>>  	return &timecounter;
>>  }
>>  
>> +/* The ARM VDSO init code needs to know:
>> + * - whether a cp15-based arch timer is present; and if so
>> + * - whether the physical or virtual counter is being used.
>> + */
>> +bool arch_timer_okay_for_vdso(void)
>> +{
>> +	if (!(arch_timers_present & ARCH_CP15_TIMER))
>> +		return false;
>> +
>> +	return arch_timer_use_virtual;
>> +}
> 
> If we're adding this, then it wouldn't hurt to use the same check in
> arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
> `current clocksource' knowledge in there too, to remove the hardcoded
> "arch_sys_counter" check from the arch code?

While I think it makes sense to consolidate the current clocksource check,
I view that as distinct from this (which needs to run at boot, before
anything uses the vdso).

I'm actually now unsure about whether the implementation I have here
is correct.  Take arch_timer_init:

static void __init arch_timer_init(void)
{
	/*
	 * 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 (is_hyp_mode_available() || !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;
		}
	}

	arch_timer_register();
	arch_timer_common_init();
}

I assume this has been working fine for arm64 up to this point --
i.e. arch_timer_use_virtual is false, but the VDSO continues to use
the virtual counter and gets correct results?  If so, I don't see any
reason this wouldn't be true for ARM.  So I'm thinking
arch_timer_use_virtual isn't the right proxy for determining
whether the VDSO can work correctly.

The reason I initially turned to arch_timer_use_virtual is in
arch_timer_of_init:

	/*
	 * If we cannot rely on firmware initializing the timer registers then
	 * we should use the physical timers instead.
	 */
	if (IS_ENABLED(CONFIG_ARM) &&
	    of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
			arch_timer_use_virtual = false;

I definitely need to catch that case, and this is currently duplicated in
arch/arm/kernel/vdso.c.  Maybe this should toggle an additional boolean,
say "arch_timer_cntvct_ok", which captures the information that is truly
of interest with respect to the VDSO using the virtual counter.

Re: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

On 04/27/2015 05:55 AM, Will Deacon wrote:
> On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
>> The 32-bit ARM VDSO needs to know whether a generic timer is present
>> and whether it is suitable for use by user space.  The VDSO
>> initialization code currently duplicates some of the logic from the
>> driver to make this determination, but unfortunately it is incomplete;
>> it will incorrectly enable the VDSO if HYP mode is available or if no
>> interrupt is provided for the virtual timer (see arch_timer_init).  In
>> these cases the driver will switch to memory-backed access while the
>> VDSO will attempt to access the counter using cp15 reads.
>>
>> Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
>> init code whether the arch timer is present and usable.
>>
>> Signed-off-by: Nathan Lynch <nathan_lynch@mentor.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 12 ++++++++++++
>>  include/clocksource/arm_arch_timer.h |  6 ++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 0aa135ddbf80..b75215523d2f 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
>>  	return &timecounter;
>>  }
>>  
>> +/* The ARM VDSO init code needs to know:
>> + * - whether a cp15-based arch timer is present; and if so
>> + * - whether the physical or virtual counter is being used.
>> + */
>> +bool arch_timer_okay_for_vdso(void)
>> +{
>> +	if (!(arch_timers_present & ARCH_CP15_TIMER))
>> +		return false;
>> +
>> +	return arch_timer_use_virtual;
>> +}
> 
> If we're adding this, then it wouldn't hurt to use the same check in
> arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
> `current clocksource' knowledge in there too, to remove the hardcoded
> "arch_sys_counter" check from the arch code?

While I think it makes sense to consolidate the current clocksource check,
I view that as distinct from this (which needs to run at boot, before
anything uses the vdso).

I'm actually now unsure about whether the implementation I have here
is correct.  Take arch_timer_init:

static void __init arch_timer_init(void)
{
	/*
	 * 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 (is_hyp_mode_available() || !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;
		}
	}

	arch_timer_register();
	arch_timer_common_init();
}

I assume this has been working fine for arm64 up to this point --
i.e. arch_timer_use_virtual is false, but the VDSO continues to use
the virtual counter and gets correct results?  If so, I don't see any
reason this wouldn't be true for ARM.  So I'm thinking
arch_timer_use_virtual isn't the right proxy for determining
whether the VDSO can work correctly.

The reason I initially turned to arch_timer_use_virtual is in
arch_timer_of_init:

	/*
	 * If we cannot rely on firmware initializing the timer registers then
	 * we should use the physical timers instead.
	 */
	if (IS_ENABLED(CONFIG_ARM) &&
	    of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
			arch_timer_use_virtual = false;

I definitely need to catch that case, and this is currently duplicated in
arch/arm/kernel/vdso.c.  Maybe this should toggle an additional boolean,
say "arch_timer_cntvct_ok", which captures the information that is truly
of interest with respect to the VDSO using the virtual counter.