From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50976C4338F for ; Sat, 7 Aug 2021 10:52:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2AD4761164 for ; Sat, 7 Aug 2021 10:52:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232026AbhHGKwg (ORCPT ); Sat, 7 Aug 2021 06:52:36 -0400 Received: from mail.kernel.org ([198.145.29.99]:55150 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231687AbhHGKwf (ORCPT ); Sat, 7 Aug 2021 06:52:35 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B6A5A60F70; Sat, 7 Aug 2021 10:52:18 +0000 (UTC) Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mCJw8-003VVW-La; Sat, 07 Aug 2021 11:52:16 +0100 Date: Sat, 07 Aug 2021 11:52:15 +0100 Message-ID: <87pmup1q8w.wl-maz@kernel.org> From: Marc Zyngier To: Oliver Upton Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Rutland , Daniel Lezcano , Thomas Gleixner , Peter Shier , Raghavendra Rao Ananta , Ricardo Koller Subject: Re: [RESEND PATCH] clocksource/arm_arch_timer: Fix masking for high freq counters In-Reply-To: <20210806182126.2842876-1-oupton@google.com> References: <20210806182126.2842876-1-oupton@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.104.136.29 X-SA-Exim-Rcpt-To: oupton@google.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mark.rutland@arm.com, daniel.lezcano@linaro.org, tglx@linutronix.de, pshier@google.com, rananta@google.com, ricarkol@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Oliver, On Fri, 06 Aug 2021 19:21:26 +0100, Oliver Upton wrote: > > Unfortunately, the architecture provides no means to determine the bit > width of the system counter. However, we do know the following from the > specification: > > - the system counter is at least 56 bits wide > - Roll-over time of not less than 40 years > > To date, the arch timer driver has depended on the first property, > assuming any system counter to be 56 bits wide and masking off the rest. > However, combining a narrow clocksource mask with a high frequency > counter could result in prematurely wrapping the system counter by a > significant margin. For example, a 56 bit wide, 1GHz system counter > would wrap in a mere 2.28 years! > > This is a problem for two reasons: v8.6+ implementations are required to > provide a 64 bit, 1GHz system counter. Furthermore, before v8.6, > implementers may select a counter frequency of their choosing. > > Fix the issue by deriving a valid clock mask based on the second > property from above. Set the floor at 56 bits, since we know no system > counter is narrower than that. > > Suggested-by: Marc Zyngier > Signed-off-by: Oliver Upton > --- > This patch was tested with QEMU, tweaked to provide a 1GHz system > counter frequency, as I could not easily figure out how to tweak the > base FVP to provide a 1GHz counter. "bp.refcounter.base_frequency" is the property you are looking for. In general, passing --list-params to the model reveals a treasure trove of weird and wonderful options that can be used to configure the model to your liking. > > Parent commit: 0c32706dac1b ("arm64: stacktrace: avoid tracing arch_stack_walk()") > > drivers/clocksource/arm_arch_timer.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index be6d741d404c..8c41626a4c8a 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -52,6 +52,12 @@ > #define CNTV_TVAL 0x38 > #define CNTV_CTL 0x3c > > +/* > + * The minimum amount of time a generic timer is guaranteed to not roll over nit: s/timer/counter/ > + * (40 years) For later reference, could you add the section of the ARMv8 ARM where this is mentioned? Something like 'ARM DDI 0487G.a D11.1.2 ("The system counter")', either here or in the comment further down. > + */ > +#define MIN_ROLLOVER_SECS (40ULL * 365 * 24 * 3600) > + > static unsigned arch_timers_present __initdata; > > static void __iomem *arch_counter_base __ro_after_init; > @@ -1004,9 +1010,24 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void) > return &arch_timer_kvm_info; > } > > +/* > + * Makes an educated guess at a valid counter width based on the Generic Timer > + * specification. Of note: > + * 1) the Generic Timer is at least 56 bits wide > + * 2) a roll-over time of not less than 40 years > + */ > +static int __init arch_counter_get_width(void) > +{ > + u64 min_cycles = MIN_ROLLOVER_SECS * arch_timer_get_cntfrq(); That's unfortunately wishful thinking. We have stupidly broken systems out there that do not set CNTFRQ_EL0, and instead rely on DT properties to describe the counter frequency. You're likely to end up with a glorious zero as a result, with interesting consequences... Use arch_timer_rate instead, which will be set as by the time you register the counter. > + > + /* guarantee the returned width is within the valid range */ > + return max(56, min(64, ilog2(min_cycles))); Maybe better written as "clamp_val(ilog2(min_cycles), 56, 64);". > +} > + > static void __init arch_counter_register(unsigned type) > { > u64 start_count; > + int width; > > /* Register the CP15 based counter if we have one */ > if (type & ARCH_TIMER_TYPE_CP15) { > @@ -1031,6 +1052,10 @@ static void __init arch_counter_register(unsigned type) > arch_timer_read_counter = arch_counter_get_cntvct_mem; > } > > + width = arch_counter_get_width(); > + clocksource_counter.mask = CLOCKSOURCE_MASK(width); > + cyclecounter.mask = CLOCKSOURCE_MASK(width); Since you move this to be computed at runtime, how about dropping the static initialisation of the mask fields? > + > if (!arch_counter_suspend_stop) > clocksource_counter.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; > start_count = arch_timer_read_counter(); > @@ -1040,8 +1065,7 @@ static void __init arch_counter_register(unsigned type) > timecounter_init(&arch_timer_kvm_info.timecounter, > &cyclecounter, start_count); > > - /* 56 bits minimum, so we assume worst case rollover */ > - sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate); > + sched_clock_register(arch_timer_read_counter, width, arch_timer_rate); > } > > static void arch_timer_stop(struct clock_event_device *clk) Another thing that needs addressing for high frequency counter support is to move away from TVAL programming and switch to CVAL, as the maximum deadline we can currently program is 4.2s at 1GHz. Fun fact: it has the interesting consequence of breaking XGene-1, which implemented CVAL in terms of TVAL instead of the other way around (what were these guys thinking?), though I don't think anyone will notice in practice. I have a preliminary patch on a branch somewhere that I'll try to dust up and post in the coming days. Thanks, M. -- Without deviation from the norm, progress is not possible. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8A3A3C4338F for ; Sat, 7 Aug 2021 10:54:04 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 192D560F70 for ; Sat, 7 Aug 2021 10:54:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 192D560F70 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xJ6UOlQTYXDCn99Z+IPiuQrcDqDLR+bsZSkVhvWqF10=; b=S0iSu1kBE2LEzI 3atY905Qcaed1+Bmki6ZWTQ/+P3EGReG4w+CpjpwR+QUmsTRo1ub6TDxLuaUAyuF7wtePbxQqiJRj 6nazJden1iiJBvamSZWYBoJLcrdGpWaC2bCMPIk7qbsTw9meKLyphdjPektsKpcEolXfjMic8POAA B2l/VUAFOgYxCgn8bMFRV2gNjuz6M3ahGElS4+W7DZ1ubSnFZy6wW8iI+6ZJUk+lZmmbSI1aWUjLN vf1++v6qarvkidZ6Dp/+1PGHNpxkvK59QqMlGR3b0+SfkivS1N4el5BIEuDiJnlhZl+JPbQq2WL6X sczAjxw907Kqs9VJ3rKg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mCJwG-00EbFZ-GX; Sat, 07 Aug 2021 10:52:24 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mCJwB-00EbEz-W6 for linux-arm-kernel@lists.infradead.org; Sat, 07 Aug 2021 10:52:21 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B6A5A60F70; Sat, 7 Aug 2021 10:52:18 +0000 (UTC) Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mCJw8-003VVW-La; Sat, 07 Aug 2021 11:52:16 +0100 Date: Sat, 07 Aug 2021 11:52:15 +0100 Message-ID: <87pmup1q8w.wl-maz@kernel.org> From: Marc Zyngier To: Oliver Upton Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Rutland , Daniel Lezcano , Thomas Gleixner , Peter Shier , Raghavendra Rao Ananta , Ricardo Koller Subject: Re: [RESEND PATCH] clocksource/arm_arch_timer: Fix masking for high freq counters In-Reply-To: <20210806182126.2842876-1-oupton@google.com> References: <20210806182126.2842876-1-oupton@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.104.136.29 X-SA-Exim-Rcpt-To: oupton@google.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mark.rutland@arm.com, daniel.lezcano@linaro.org, tglx@linutronix.de, pshier@google.com, rananta@google.com, ricarkol@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210807_035220_117640_4643701B X-CRM114-Status: GOOD ( 40.10 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Oliver, On Fri, 06 Aug 2021 19:21:26 +0100, Oliver Upton wrote: > > Unfortunately, the architecture provides no means to determine the bit > width of the system counter. However, we do know the following from the > specification: > > - the system counter is at least 56 bits wide > - Roll-over time of not less than 40 years > > To date, the arch timer driver has depended on the first property, > assuming any system counter to be 56 bits wide and masking off the rest. > However, combining a narrow clocksource mask with a high frequency > counter could result in prematurely wrapping the system counter by a > significant margin. For example, a 56 bit wide, 1GHz system counter > would wrap in a mere 2.28 years! > > This is a problem for two reasons: v8.6+ implementations are required to > provide a 64 bit, 1GHz system counter. Furthermore, before v8.6, > implementers may select a counter frequency of their choosing. > > Fix the issue by deriving a valid clock mask based on the second > property from above. Set the floor at 56 bits, since we know no system > counter is narrower than that. > > Suggested-by: Marc Zyngier > Signed-off-by: Oliver Upton > --- > This patch was tested with QEMU, tweaked to provide a 1GHz system > counter frequency, as I could not easily figure out how to tweak the > base FVP to provide a 1GHz counter. "bp.refcounter.base_frequency" is the property you are looking for. In general, passing --list-params to the model reveals a treasure trove of weird and wonderful options that can be used to configure the model to your liking. > > Parent commit: 0c32706dac1b ("arm64: stacktrace: avoid tracing arch_stack_walk()") > > drivers/clocksource/arm_arch_timer.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index be6d741d404c..8c41626a4c8a 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -52,6 +52,12 @@ > #define CNTV_TVAL 0x38 > #define CNTV_CTL 0x3c > > +/* > + * The minimum amount of time a generic timer is guaranteed to not roll over nit: s/timer/counter/ > + * (40 years) For later reference, could you add the section of the ARMv8 ARM where this is mentioned? Something like 'ARM DDI 0487G.a D11.1.2 ("The system counter")', either here or in the comment further down. > + */ > +#define MIN_ROLLOVER_SECS (40ULL * 365 * 24 * 3600) > + > static unsigned arch_timers_present __initdata; > > static void __iomem *arch_counter_base __ro_after_init; > @@ -1004,9 +1010,24 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void) > return &arch_timer_kvm_info; > } > > +/* > + * Makes an educated guess at a valid counter width based on the Generic Timer > + * specification. Of note: > + * 1) the Generic Timer is at least 56 bits wide > + * 2) a roll-over time of not less than 40 years > + */ > +static int __init arch_counter_get_width(void) > +{ > + u64 min_cycles = MIN_ROLLOVER_SECS * arch_timer_get_cntfrq(); That's unfortunately wishful thinking. We have stupidly broken systems out there that do not set CNTFRQ_EL0, and instead rely on DT properties to describe the counter frequency. You're likely to end up with a glorious zero as a result, with interesting consequences... Use arch_timer_rate instead, which will be set as by the time you register the counter. > + > + /* guarantee the returned width is within the valid range */ > + return max(56, min(64, ilog2(min_cycles))); Maybe better written as "clamp_val(ilog2(min_cycles), 56, 64);". > +} > + > static void __init arch_counter_register(unsigned type) > { > u64 start_count; > + int width; > > /* Register the CP15 based counter if we have one */ > if (type & ARCH_TIMER_TYPE_CP15) { > @@ -1031,6 +1052,10 @@ static void __init arch_counter_register(unsigned type) > arch_timer_read_counter = arch_counter_get_cntvct_mem; > } > > + width = arch_counter_get_width(); > + clocksource_counter.mask = CLOCKSOURCE_MASK(width); > + cyclecounter.mask = CLOCKSOURCE_MASK(width); Since you move this to be computed at runtime, how about dropping the static initialisation of the mask fields? > + > if (!arch_counter_suspend_stop) > clocksource_counter.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; > start_count = arch_timer_read_counter(); > @@ -1040,8 +1065,7 @@ static void __init arch_counter_register(unsigned type) > timecounter_init(&arch_timer_kvm_info.timecounter, > &cyclecounter, start_count); > > - /* 56 bits minimum, so we assume worst case rollover */ > - sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate); > + sched_clock_register(arch_timer_read_counter, width, arch_timer_rate); > } > > static void arch_timer_stop(struct clock_event_device *clk) Another thing that needs addressing for high frequency counter support is to move away from TVAL programming and switch to CVAL, as the maximum deadline we can currently program is 4.2s at 1GHz. Fun fact: it has the interesting consequence of breaking XGene-1, which implemented CVAL in terms of TVAL instead of the other way around (what were these guys thinking?), though I don't think anyone will notice in practice. I have a preliminary patch on a branch somewhere that I'll try to dust up and post in the coming days. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel