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_ly...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
