| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230602115451.GG620383@hirez.programming.kicks-ass.net>
Date: Fri, 2 Jun 2023 13:54:51 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Valentin Schneider <vschneid@...hat.com>
Cc: bigeasy@...utronix.de, mark.rutland@....com, maz@...nel.org,
catalin.marinas@....com, will@...nel.org, chenhuacai@...nel.org,
kernel@...0n.name, hca@...ux.ibm.com, gor@...ux.ibm.com,
agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
svens@...ux.ibm.com, pbonzini@...hat.com, wanpengli@...cent.com,
vkuznets@...hat.com, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org,
hpa@...or.com, jgross@...e.com, boris.ostrovsky@...cle.com,
daniel.lezcano@...aro.org, kys@...rosoft.com,
haiyangz@...rosoft.com, wei.liu@...nel.org, decui@...rosoft.com,
rafael@...nel.org, longman@...hat.com, boqun.feng@...il.com,
pmladek@...e.com, senozhatsky@...omium.org, rostedt@...dmis.org,
john.ogness@...utronix.de, juri.lelli@...hat.com,
vincent.guittot@...aro.org, dietmar.eggemann@....com,
bsegall@...gle.com, mgorman@...e.de, bristot@...hat.com,
jstultz@...gle.com, sboyd@...nel.org, linux-kernel@...r.kernel.org,
loongarch@...ts.linux.dev, linux-s390@...r.kernel.org,
kvm@...r.kernel.org, linux-hyperv@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 04/13] arm64/arch_timer: Provide noinstr
sched_clock_read() functions
On Wed, May 24, 2023 at 05:40:47PM +0100, Valentin Schneider wrote:
> On 19/05/23 12:21, Peter Zijlstra wrote:
> > With the intent to provide local_clock_noinstr(), a variant of
> > local_clock() that's safe to be called from noinstr code (with the
> > assumption that any such code will already be non-preemptible),
> > prepare for things by providing a noinstr sched_clock_read() function.
> >
> > Specifically, preempt_enable_*() calls out to schedule(), which upsets
> > noinstr validation efforts.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > ---
> > arch/arm64/include/asm/arch_timer.h | 8 ----
> > drivers/clocksource/arm_arch_timer.c | 60 ++++++++++++++++++++++++++---------
> > 2 files changed, 47 insertions(+), 21 deletions(-)
> >
> > --- a/arch/arm64/include/asm/arch_timer.h
> > +++ b/arch/arm64/include/asm/arch_timer.h
> > @@ -88,13 +88,7 @@ static inline notrace u64 arch_timer_rea
> >
> > #define arch_timer_reg_read_stable(reg) \
> > ({ \
> > - u64 _val; \
> > - \
> > - preempt_disable_notrace(); \
> > - _val = erratum_handler(read_ ## reg)(); \
> > - preempt_enable_notrace(); \
> > - \
> > - _val; \
> > + erratum_handler(read_ ## reg)(); \
> > })
> >
> > /*
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -191,22 +191,40 @@ u32 arch_timer_reg_read(int access, enum
> > return val;
> > }
> >
> > -static notrace u64 arch_counter_get_cntpct_stable(void)
> > +static noinstr u64 _arch_counter_get_cntpct_stable(void)
> > {
> > return __arch_counter_get_cntpct_stable();
> > }
> >
>
> I tripped over the underscores when reviewing this :( This must be
> getting close to the symbol length limit, but could we give this a helpful
> prefix or suffix? raw_* or *_noinstr?
>
> > @@ -1074,6 +1092,13 @@ struct arch_timer_kvm_info *arch_timer_g
> >
> > static void __init arch_counter_register(unsigned type)
> > {
> > + /*
> > + * Default to cp15 based access because arm64 uses this function for
> > + * sched_clock() before DT is probed and the cp15 method is guaranteed
> > + * to exist on arm64. arm doesn't use this before DT is probed so even
> > + * if we don't have the cp15 accessors we won't have a problem.
> > + */
> > + u64 (*scr)(void) = arch_counter_get_cntvct;
>
> So this bit sent me on a little spelunking session :-)
>
> From a control flow perspective the initialization isn't required, but then
> I looked into the comment and found it comes from the
> arch_timer_read_counter() definition... Which itself doesn't get used by
> sched_clock() until the sched_clock_register() below!
>
> So AFAICT that comment was true as of
>
> 220069945b29 ("clocksource: arch_timer: Add support for memory mapped timers")
>
> but not after a commit that came 2 months later:
>
> 65cd4f6c99c1 ("arch_timer: Move to generic sched_clock framework")
>
> which IIUC made arm/arm64 follow the default approach of using the
> jiffy-based sched_clock() before probing DT/ACPI and registering a "proper"
> sched_clock.
>
> All of that to say: the comment about arch_timer_read_counter() vs early
> sched_clock() doesn't apply anymore, but I think we need to keep its
> initalization around for stuff like get_cycles(). This initialization here
> should be OK to put to the bin, though.
Something like the below folded in then?
---
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -191,7 +191,7 @@ u32 arch_timer_reg_read(int access, enum
return val;
}
-static noinstr u64 _arch_counter_get_cntpct_stable(void)
+static noinstr u64 raw_counter_get_cntpct_stable(void)
{
return __arch_counter_get_cntpct_stable();
}
@@ -210,7 +210,7 @@ static noinstr u64 arch_counter_get_cntp
return __arch_counter_get_cntpct();
}
-static noinstr u64 _arch_counter_get_cntvct_stable(void)
+static noinstr u64 raw_counter_get_cntvct_stable(void)
{
return __arch_counter_get_cntvct_stable();
}
@@ -1092,13 +1092,7 @@ struct arch_timer_kvm_info *arch_timer_g
static void __init arch_counter_register(unsigned type)
{
- /*
- * Default to cp15 based access because arm64 uses this function for
- * sched_clock() before DT is probed and the cp15 method is guaranteed
- * to exist on arm64. arm doesn't use this before DT is probed so even
- * if we don't have the cp15 accessors we won't have a problem.
- */
- u64 (*scr)(void) = arch_counter_get_cntvct;
+ u64 (*scr)(void);
u64 start_count;
int width;
@@ -1110,7 +1104,7 @@ static void __init arch_counter_register
arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
if (arch_timer_counter_has_wa()) {
rd = arch_counter_get_cntvct_stable;
- scr = _arch_counter_get_cntvct_stable;
+ scr = raw_counter_get_cntvct_stable;
} else {
rd = arch_counter_get_cntvct;
scr = arch_counter_get_cntvct;
@@ -1118,7 +1112,7 @@ static void __init arch_counter_register
} else {
if (arch_timer_counter_has_wa()) {
rd = arch_counter_get_cntpct_stable;
- scr = _arch_counter_get_cntpct_stable;
+ scr = raw_counter_get_cntpct_stable;
} else {
rd = arch_counter_get_cntpct;
scr = arch_counter_get_cntpct;
Powered by blists - more mailing lists