From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nathan_Lynch@mentor.com (Nathan Lynch) Date: Tue, 28 Apr 2015 09:08:43 -0500 Subject: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso In-Reply-To: <20150427105509.GC1544@arm.com> References: <1429911801-6069-1-git-send-email-nathan_lynch@mentor.com> <20150427105509.GC1544@arm.com> Message-ID: <553F946B.7030306@mentor.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> --- >> 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030822AbbD1OIw (ORCPT ); Tue, 28 Apr 2015 10:08:52 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:60897 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030210AbbD1OIu (ORCPT ); Tue, 28 Apr 2015 10:08:50 -0400 Message-ID: <553F946B.7030306@mentor.com> Date: Tue, 28 Apr 2015 09:08:43 -0500 From: Nathan Lynch User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Will Deacon CC: "linux-arm-kernel@lists.infradead.org" , Daniel Lezcano , Catalin Marinas , Doug Anderson , Marc Zyngier , Mark Rutland , Russell King , Sonny Rao , Stephen Boyd , Thomas Gleixner , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso References: <1429911801-6069-1-git-send-email-nathan_lynch@mentor.com> <20150427105509.GC1544@arm.com> In-Reply-To: <20150427105509.GC1544@arm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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.