[RFC][PATCH 0/9] local_clock() vs noinstr

[RFC][PATCH 0/9] local_clock() vs noinstr

[Peter Zijlstra]

Hi all!

A recent commit of mine marked local_clock() as noinstr.

  776f22913b8e ("sched/clock: Make local_clock() noinstr")

Sadly both me and objtool missed the fact that this is subly broken; but
Sebastian tripped over it [*]:

| vmlinux.o: warning: objtool: native_sched_clock+0x97: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
| vmlinux.o: warning: objtool: local_clock+0xb4: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section

Specifically, local_clock() (and many of the sched_clock() implementation is
relies upon) use preempt_{dis,en}able_notrace() which obviously calls out to
schedule().

Now, noinstr code *should* never trigger this and already run in
non-preemptible code. Specifically entry code should have IRQs disabled while
__cpuidle code should have preemption disabled.

So while it is mostly harmless, I figured it wouldn't be too hard to clean this
up a little -- but that was ~10 patches. Anyway, here goes...

Compile tested only on x86_64/s390/arm64 -- I've just fed it to the
robots.

---
 arch/arm64/include/asm/arch_timer.h    |  8 +----
 arch/arm64/include/asm/io.h            | 12 +++----
 arch/loongarch/include/asm/loongarch.h |  2 +-
 arch/loongarch/kernel/time.c           |  6 ++--
 arch/s390/include/asm/timex.h          | 13 +++++---
 arch/s390/kernel/time.c                | 11 ++++++-
 arch/x86/kernel/kvmclock.c             |  4 +--
 arch/x86/kernel/tsc.c                  | 38 ++++++++++++++++-----
 arch/x86/xen/time.c                    |  3 +-
 drivers/clocksource/arm_arch_timer.c   | 60 ++++++++++++++++++++++++++--------
 drivers/clocksource/hyperv_timer.c     |  4 +--
 drivers/cpuidle/cpuidle.c              |  8 ++---
 drivers/cpuidle/poll_state.c           |  4 +--
 include/clocksource/hyperv_timer.h     |  4 +--
 include/linux/rbtree_latch.h           |  2 +-
 include/linux/sched/clock.h            | 17 +++++++++-
 include/linux/seqlock.h                | 15 +++++----
 kernel/printk/printk.c                 |  2 +-
 kernel/sched/clock.c                   | 19 +++++++----
 kernel/time/sched_clock.c              | 24 ++++++++++----
 kernel/time/timekeeping.c              |  4 +--
 21 files changed, 176 insertions(+), 84 deletions(-)


* https://lkml.kernel.org/r/20230309072724.3F6zRkvw@linutronix.de
  TL;DR: PREEMPT_DYNAMIC=n PREEMPT=y DEBUG_ENTRY=y

[RFC][PATCH 1/9] seqlock/latch: Provide raw_read_seqcount_latch_retry()

[Peter Zijlstra]

The read side of seqcount_latch consists of:

  do {
    seq = raw_read_seqcount_latch(&latch->seq);
    ...
  } while (read_seqcount_latch_retry(&latch->seq, seq));

which is asymmetric in the raw_ department, and sure enough,
read_seqcount_latch_retry() includes (explicit) instrumentation where
raw_read_seqcount_latch() does not.

This inconsistency becomes a problem when trying to use it from
noinstr code. As such, fix it by renaming and re-implementing
raw_read_seqcount_latch_retry() without the instrumentation.

Specifically the instrumentation in question is kcsan_atomic_next(0)
in do___read_seqcount_retry(). Loosing this annotation is not a
problem because raw_read_seqcount_latch() does not pass through
kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/rbtree_latch.h |    2 +-
 include/linux/seqlock.h      |   15 ++++++++-------
 kernel/printk/printk.c       |    2 +-
 kernel/time/sched_clock.c    |    2 +-
 kernel/time/timekeeping.c    |    4 ++--
 5 files changed, 13 insertions(+), 12 deletions(-)

--- a/include/linux/rbtree_latch.h
+++ b/include/linux/rbtree_latch.h
@@ -206,7 +206,7 @@ latch_tree_find(void *key, struct latch_
 	do {
 		seq = raw_read_seqcount_latch(&root->seq);
 		node = __lt_find(key, root, seq & 1, ops->comp);
-	} while (read_seqcount_latch_retry(&root->seq, seq));
+	} while (raw_read_seqcount_latch_retry(&root->seq, seq));
 
 	return node;
 }
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -671,9 +671,9 @@ typedef struct {
  *
  * Return: sequence counter raw value. Use the lowest bit as an index for
  * picking which data copy to read. The full counter must then be checked
- * with read_seqcount_latch_retry().
+ * with raw_read_seqcount_latch_retry().
  */
-static inline unsigned raw_read_seqcount_latch(const seqcount_latch_t *s)
+static __always_inline unsigned raw_read_seqcount_latch(const seqcount_latch_t *s)
 {
 	/*
 	 * Pairs with the first smp_wmb() in raw_write_seqcount_latch().
@@ -683,16 +683,17 @@ static inline unsigned raw_read_seqcount
 }
 
 /**
- * read_seqcount_latch_retry() - end a seqcount_latch_t read section
+ * raw_read_seqcount_latch_retry() - end a seqcount_latch_t read section
  * @s:		Pointer to seqcount_latch_t
  * @start:	count, from raw_read_seqcount_latch()
  *
  * Return: true if a read section retry is required, else false
  */
-static inline int
-read_seqcount_latch_retry(const seqcount_latch_t *s, unsigned start)
+static __always_inline int
+raw_read_seqcount_latch_retry(const seqcount_latch_t *s, unsigned start)
 {
-	return read_seqcount_retry(&s->seqcount, start);
+	smp_rmb();
+	return unlikely(READ_ONCE(s->seqcount.sequence) != start);
 }
 
 /**
@@ -752,7 +753,7 @@ read_seqcount_latch_retry(const seqcount
  *			entry = data_query(latch->data[idx], ...);
  *
  *		// This includes needed smp_rmb()
- *		} while (read_seqcount_latch_retry(&latch->seq, seq));
+ *		} while (raw_read_seqcount_latch_retry(&latch->seq, seq));
  *
  *		return entry;
  *	}
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -597,7 +597,7 @@ static u64 latched_seq_read_nolock(struc
 		seq = raw_read_seqcount_latch(&ls->latch);
 		idx = seq & 0x1;
 		val = ls->val[idx];
-	} while (read_seqcount_latch_retry(&ls->latch, seq));
+	} while (raw_read_seqcount_latch_retry(&ls->latch, seq));
 
 	return val;
 }
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -77,7 +77,7 @@ notrace struct clock_read_data *sched_cl
 
 notrace int sched_clock_read_retry(unsigned int seq)
 {
-	return read_seqcount_latch_retry(&cd.seq, seq);
+	return raw_read_seqcount_latch_retry(&cd.seq, seq);
 }
 
 unsigned long long notrace sched_clock(void)
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -450,7 +450,7 @@ static __always_inline u64 __ktime_get_f
 		tkr = tkf->base + (seq & 0x01);
 		now = ktime_to_ns(tkr->base);
 		now += fast_tk_get_delta_ns(tkr);
-	} while (read_seqcount_latch_retry(&tkf->seq, seq));
+	} while (raw_read_seqcount_latch_retry(&tkf->seq, seq));
 
 	return now;
 }
@@ -566,7 +566,7 @@ static __always_inline u64 __ktime_get_r
 		basem = ktime_to_ns(tkr->base);
 		baser = ktime_to_ns(tkr->base_real);
 		delta = fast_tk_get_delta_ns(tkr);
-	} while (read_seqcount_latch_retry(&tkf->seq, seq));
+	} while (raw_read_seqcount_latch_retry(&tkf->seq, seq));
 
 	if (mono)
 		*mono = basem + delta;

Re: [RFC][PATCH 1/9] seqlock/latch: Provide raw_read_seqcount_latch_retry()

[Thomas Gleixner]

On Mon, May 08 2023 at 23:19, Peter Zijlstra wrote:
> The read side of seqcount_latch consists of:
>
>   do {
>     seq = raw_read_seqcount_latch(&latch->seq);
>     ...
>   } while (read_seqcount_latch_retry(&latch->seq, seq));
>
> which is asymmetric in the raw_ department, and sure enough,
> read_seqcount_latch_retry() includes (explicit) instrumentation where
> raw_read_seqcount_latch() does not.
>
> This inconsistency becomes a problem when trying to use it from
> noinstr code. As such, fix it by renaming and re-implementing
> raw_read_seqcount_latch_retry() without the instrumentation.
>
> Specifically the instrumentation in question is kcsan_atomic_next(0)
> in do___read_seqcount_retry(). Loosing this annotation is not a
> problem because raw_read_seqcount_latch() does not pass through
> kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX).
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [RFC][PATCH 1/9] seqlock/latch: Provide raw_read_seqcount_latch_retry()

[Petr Mladek]

On Mon 2023-05-08 23:19:52, Peter Zijlstra wrote:
> The read side of seqcount_latch consists of:
> 
>   do {
>     seq = raw_read_seqcount_latch(&latch->seq);
>     ...
>   } while (read_seqcount_latch_retry(&latch->seq, seq));
> 
> which is asymmetric in the raw_ department, and sure enough,
> read_seqcount_latch_retry() includes (explicit) instrumentation where
> raw_read_seqcount_latch() does not.
> 
> This inconsistency becomes a problem when trying to use it from
> noinstr code. As such, fix it by renaming and re-implementing
> raw_read_seqcount_latch_retry() without the instrumentation.
> 
> Specifically the instrumentation in question is kcsan_atomic_next(0)
> in do___read_seqcount_retry(). Loosing this annotation is not a
> problem because raw_read_seqcount_latch() does not pass through
> kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX).
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Makes sense:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[RFC][PATCH 2/9] time/sched_clock: Provide sched_clock_noinstr()

[Peter Zijlstra]

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_noinstr() function.

Specifically, preempt_enable_*() calls out to schedule(), which upsets
noinstr validation efforts.

As such, pull out the preempt_{dis,en}able_notrace() requirements from
the sched_clock_read() implementations by explicitly providing it in
the sched_clock() function.

This further requires said sched_clock_read() functions to be noinstr
themselves, for ARCH_WANTS_NO_INSTR users. See the next few patches.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/time/sched_clock.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -64,7 +64,7 @@ static struct clock_data cd ____cachelin
 	.actual_read_sched_clock = jiffy_sched_clock_read,
 };
 
-static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
+static __always_inline u64 cyc_to_ns(u64 cyc, u32 mult, u32 shift)
 {
 	return (cyc * mult) >> shift;
 }
@@ -80,23 +80,33 @@ notrace int sched_clock_read_retry(unsig
 	return raw_read_seqcount_latch_retry(&cd.seq, seq);
 }
 
-unsigned long long notrace sched_clock(void)
+unsigned long long noinstr sched_clock_noinstr(void)
 {
-	u64 cyc, res;
-	unsigned int seq;
 	struct clock_read_data *rd;
+	unsigned int seq;
+	u64 cyc, res;
 
 	do {
-		rd = sched_clock_read_begin(&seq);
+		seq = raw_read_seqcount_latch(&cd.seq);
+		rd = cd.read_data + (seq & 1);
 
 		cyc = (rd->read_sched_clock() - rd->epoch_cyc) &
 		      rd->sched_clock_mask;
 		res = rd->epoch_ns + cyc_to_ns(cyc, rd->mult, rd->shift);
-	} while (sched_clock_read_retry(seq));
+	} while (raw_read_seqcount_latch_retry(&cd.seq, seq));
 
 	return res;
 }
 
+unsigned long long notrace sched_clock(void)
+{
+	unsigned long long ns;
+	preempt_disable_notrace();
+	ns = sched_clock_noinstr();
+	preempt_enable_notrace();
+	return ns;
+}
+
 /*
  * Updating the data required to read the clock.
  *

[RFC][PATCH 3/9] arm64/io: Always inline all of __raw_{read,write}[bwlq]()

[Peter Zijlstra]

The next patch will want to use __raw_readl() from a noinstr section
and as such that needs to be marked __always_inline to avoid the
compiler being a silly bugger.

Turns out it already is, but its siblings are not.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm64/include/asm/io.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -22,13 +22,13 @@
  * Generic IO read/write.  These perform native-endian accesses.
  */
 #define __raw_writeb __raw_writeb
-static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
+static __always_inline void __raw_writeb(u8 val, volatile void __iomem *addr)
 {
 	asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
 #define __raw_writew __raw_writew
-static inline void __raw_writew(u16 val, volatile void __iomem *addr)
+static __always_inline void __raw_writew(u16 val, volatile void __iomem *addr)
 {
 	asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr));
 }
@@ -40,13 +40,13 @@ static __always_inline void __raw_writel
 }
 
 #define __raw_writeq __raw_writeq
-static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
+static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)
 {
 	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
 }
 
 #define __raw_readb __raw_readb
-static inline u8 __raw_readb(const volatile void __iomem *addr)
+static __always_inline u8 __raw_readb(const volatile void __iomem *addr)
 {
 	u8 val;
 	asm volatile(ALTERNATIVE("ldrb %w0, [%1]",
@@ -57,7 +57,7 @@ static inline u8 __raw_readb(const volat
 }
 
 #define __raw_readw __raw_readw
-static inline u16 __raw_readw(const volatile void __iomem *addr)
+static __always_inline u16 __raw_readw(const volatile void __iomem *addr)
 {
 	u16 val;
 
@@ -80,7 +80,7 @@ static __always_inline u32 __raw_readl(c
 }
 
 #define __raw_readq __raw_readq
-static inline u64 __raw_readq(const volatile void __iomem *addr)
+static __always_inline u64 __raw_readq(const volatile void __iomem *addr)
 {
 	u64 val;
 	asm volatile(ALTERNATIVE("ldr %0, [%1]",

Re: [RFC][PATCH 3/9] arm64/io: Always inline all of __raw_{read,write}[bwlq]()

[Mark Rutland]

On Mon, May 08, 2023 at 11:19:54PM +0200, Peter Zijlstra wrote:
> The next patch will want to use __raw_readl() from a noinstr section
> and as such that needs to be marked __always_inline to avoid the
> compiler being a silly bugger.
> 
> Turns out it already is, but its siblings are not.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

FWIW, on arm64 we shouldn't ever end up with the MMIO counter being our
{sched,local}_clock() -- several things blow up if we don't have the "CP15"
version, and if we do have the CP15 version we'll use that as our preferred
clocksource (and yes, the code is a mess).

Regardless, for consistency I agree we should mark these all as __always_inline.

It looks like we marked __raw_{readl,writel}() as noinstr in commit:

  e43f1331e2ef913b ("arm64: Ask the compiler to __always_inline functions used by KVM at HYP")

... and it'd be nice to mention that in the commit message.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/io.h |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -22,13 +22,13 @@
>   * Generic IO read/write.  These perform native-endian accesses.
>   */
>  #define __raw_writeb __raw_writeb
> -static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
> +static __always_inline void __raw_writeb(u8 val, volatile void __iomem *addr)
>  {
>  	asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>  }
>  
>  #define __raw_writew __raw_writew
> -static inline void __raw_writew(u16 val, volatile void __iomem *addr)
> +static __always_inline void __raw_writew(u16 val, volatile void __iomem *addr)
>  {
>  	asm volatile("strh %w0, [%1]" : : "rZ" (val), "r" (addr));
>  }
> @@ -40,13 +40,13 @@ static __always_inline void __raw_writel
>  }
>  
>  #define __raw_writeq __raw_writeq
> -static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> +static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)
>  {
>  	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
>  }
>  
>  #define __raw_readb __raw_readb
> -static inline u8 __raw_readb(const volatile void __iomem *addr)
> +static __always_inline u8 __raw_readb(const volatile void __iomem *addr)
>  {
>  	u8 val;
>  	asm volatile(ALTERNATIVE("ldrb %w0, [%1]",
> @@ -57,7 +57,7 @@ static inline u8 __raw_readb(const volat
>  }
>  
>  #define __raw_readw __raw_readw
> -static inline u16 __raw_readw(const volatile void __iomem *addr)
> +static __always_inline u16 __raw_readw(const volatile void __iomem *addr)
>  {
>  	u16 val;
>  
> @@ -80,7 +80,7 @@ static __always_inline u32 __raw_readl(c
>  }
>  
>  #define __raw_readq __raw_readq
> -static inline u64 __raw_readq(const volatile void __iomem *addr)
> +static __always_inline u64 __raw_readq(const volatile void __iomem *addr)
>  {
>  	u64 val;
>  	asm volatile(ALTERNATIVE("ldr %0, [%1]",
> 
> 

[RFC][PATCH 4/9] arm64/arch_timer: Provide noinstr sched_clock_read() functions

[Peter Zijlstra]

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@infradead.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();
 }
 
-static notrace u64 arch_counter_get_cntpct(void)
+static notrace u64 arch_counter_get_cntpct_stable(void)
+{
+	u64 val;
+	preempt_disable_notrace();
+	val = __arch_counter_get_cntpct_stable();
+	preempt_enable_notrace();
+	return val;
+}
+
+static noinstr u64 arch_counter_get_cntpct(void)
 {
 	return __arch_counter_get_cntpct();
 }
 
-static notrace u64 arch_counter_get_cntvct_stable(void)
+static noinstr u64 _arch_counter_get_cntvct_stable(void)
 {
 	return __arch_counter_get_cntvct_stable();
 }
 
-static notrace u64 arch_counter_get_cntvct(void)
+static notrace u64 arch_counter_get_cntvct_stable(void)
+{
+	u64 val;
+	preempt_disable_notrace();
+	val = __arch_counter_get_cntvct_stable();
+	preempt_enable_notrace();
+	return val;
+}
+
+static noinstr u64 arch_counter_get_cntvct(void)
 {
 	return __arch_counter_get_cntvct();
 }
@@ -753,14 +771,14 @@ static int arch_timer_set_next_event_phy
 	return 0;
 }
 
-static u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
+static noinstr u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
 {
 	u32 cnt_lo, cnt_hi, tmp_hi;
 
 	do {
-		cnt_hi = readl_relaxed(t->base + offset_lo + 4);
-		cnt_lo = readl_relaxed(t->base + offset_lo);
-		tmp_hi = readl_relaxed(t->base + offset_lo + 4);
+		cnt_hi = __raw_readl(t->base + offset_lo + 4);
+		cnt_lo = __raw_readl(t->base + offset_lo);
+		tmp_hi = __raw_readl(t->base + offset_lo + 4);
 	} while (cnt_hi != tmp_hi);
 
 	return ((u64) cnt_hi << 32) | cnt_lo;
@@ -1060,7 +1078,7 @@ bool arch_timer_evtstrm_available(void)
 	return cpumask_test_cpu(raw_smp_processor_id(), &evtstrm_available);
 }
 
-static u64 arch_counter_get_cntvct_mem(void)
+static noinstr u64 arch_counter_get_cntvct_mem(void)
 {
 	return arch_counter_get_cnt_mem(arch_timer_mem, CNTVCT_LO);
 }
@@ -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;
 	u64 start_count;
 	int width;
 
@@ -1083,21 +1108,28 @@ static void __init arch_counter_register
 
 		if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
 		    arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
-			if (arch_timer_counter_has_wa())
+			if (arch_timer_counter_has_wa()) {
 				rd = arch_counter_get_cntvct_stable;
-			else
+				scr = _arch_counter_get_cntvct_stable;
+			} else {
 				rd = arch_counter_get_cntvct;
+				scr = arch_counter_get_cntvct;
+			}
 		} else {
-			if (arch_timer_counter_has_wa())
+			if (arch_timer_counter_has_wa()) {
 				rd = arch_counter_get_cntpct_stable;
-			else
+				scr = _arch_counter_get_cntpct_stable;
+			} else {
 				rd = arch_counter_get_cntpct;
+				scr = arch_counter_get_cntpct;
+			}
 		}
 
 		arch_timer_read_counter = rd;
 		clocksource_counter.vdso_clock_mode = vdso_default;
 	} else {
 		arch_timer_read_counter = arch_counter_get_cntvct_mem;
+		scr = arch_counter_get_cntvct_mem;
 	}
 
 	width = arch_counter_get_width();
@@ -1113,7 +1145,7 @@ static void __init arch_counter_register
 	timecounter_init(&arch_timer_kvm_info.timecounter,
 			 &cyclecounter, start_count);
 
-	sched_clock_register(arch_timer_read_counter, width, arch_timer_rate);
+	sched_clock_register(scr, width, arch_timer_rate);
 }
 
 static void arch_timer_stop(struct clock_event_device *clk)

[RFC][PATCH 5/9] loongarch: Provide noinstr sched_clock_read()

[Peter Zijlstra]

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@infradead.org>
---
 arch/loongarch/include/asm/loongarch.h |    2 +-
 arch/loongarch/kernel/time.c           |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

--- a/arch/loongarch/include/asm/loongarch.h
+++ b/arch/loongarch/include/asm/loongarch.h
@@ -1167,7 +1167,7 @@ static __always_inline void iocsr_write6
 
 #ifndef __ASSEMBLY__
 
-static inline u64 drdtime(void)
+static __always_inline u64 drdtime(void)
 {
 	int rID = 0;
 	u64 val = 0;
--- a/arch/loongarch/kernel/time.c
+++ b/arch/loongarch/kernel/time.c
@@ -190,9 +190,9 @@ static u64 read_const_counter(struct clo
 	return drdtime();
 }
 
-static u64 native_sched_clock(void)
+static noinstr u64 sched_clock_read(void)
 {
-	return read_const_counter(NULL);
+	return drdtime();
 }
 
 static struct clocksource clocksource_const = {
@@ -211,7 +211,7 @@ int __init constant_clocksource_init(voi
 
 	res = clocksource_register_hz(&clocksource_const, freq);
 
-	sched_clock_register(native_sched_clock, 64, freq);
+	sched_clock_register(sched_clock_read, 64, freq);
 
 	pr_info("Constant clock source device register\n");
 

[RFC][PATCH 6/9] s390/time: Provide sched_clock_noinstr()

[Peter Zijlstra]

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_noinstr()
function.

Specifically, preempt_enable_*() calls out to schedule(), which upsets
noinstr validation efforts.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/s390/include/asm/timex.h |   13 +++++++++----
 arch/s390/kernel/time.c       |   11 ++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

--- a/arch/s390/include/asm/timex.h
+++ b/arch/s390/include/asm/timex.h
@@ -63,7 +63,7 @@ static inline int store_tod_clock_ext_cc
 	return cc;
 }
 
-static inline void store_tod_clock_ext(union tod_clock *tod)
+static __always_inline void store_tod_clock_ext(union tod_clock *tod)
 {
 	asm volatile("stcke %0" : "=Q" (*tod) : : "cc");
 }
@@ -177,7 +177,7 @@ static inline void local_tick_enable(uns
 
 typedef unsigned long cycles_t;
 
-static inline unsigned long get_tod_clock(void)
+static __always_inline unsigned long get_tod_clock(void)
 {
 	union tod_clock clk;
 
@@ -204,6 +204,11 @@ void init_cpu_timer(void);
 
 extern union tod_clock tod_clock_base;
 
+static __always_inline unsigned long __get_tod_clock_monotonic(void)
+{
+	return get_tod_clock() - tod_clock_base.tod;
+}
+
 /**
  * get_clock_monotonic - returns current time in clock rate units
  *
@@ -216,7 +221,7 @@ static inline unsigned long get_tod_cloc
 	unsigned long tod;
 
 	preempt_disable_notrace();
-	tod = get_tod_clock() - tod_clock_base.tod;
+	tod = __get_tod_clock_monotonic();
 	preempt_enable_notrace();
 	return tod;
 }
@@ -240,7 +245,7 @@ static inline unsigned long get_tod_cloc
  * -> ns = (th * 125) + ((tl * 125) >> 9);
  *
  */
-static inline unsigned long tod_to_ns(unsigned long todval)
+static __always_inline unsigned long tod_to_ns(unsigned long todval)
 {
 	return ((todval >> 9) * 125) + (((todval & 0x1ff) * 125) >> 9);
 }
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -102,12 +102,21 @@ void __init time_early_init(void)
 			((long) qui.old_leap * 4096000000L);
 }
 
+unsigned long long noinstr sched_clock_noinstr(void)
+{
+	return tod_to_ns(__get_tod_clock_monotonic());
+}
+
 /*
  * Scheduler clock - returns current time in nanosec units.
  */
 unsigned long long notrace sched_clock(void)
 {
-	return tod_to_ns(get_tod_clock_monotonic());
+	unsigned long long ns;
+	preempt_disable_notrace();
+	ns = tod_to_ns(get_tod_clock_monotonic());
+	preempt_enable_notrace();
+	return ns;
 }
 NOKPROBE_SYMBOL(sched_clock);
 

Re: [RFC][PATCH 6/9] s390/time: Provide sched_clock_noinstr()

[Heiko Carstens]

1;115;0cOn Mon, May 08, 2023 at 11:19:57PM +0200, 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_noinstr()
> function.
> 
> Specifically, preempt_enable_*() calls out to schedule(), which upsets
> noinstr validation efforts.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/s390/include/asm/timex.h |   13 +++++++++----
>  arch/s390/kernel/time.c       |   11 ++++++++++-
>  2 files changed, 19 insertions(+), 5 deletions(-)
...
> +static __always_inline unsigned long __get_tod_clock_monotonic(void)
> +{
> +	return get_tod_clock() - tod_clock_base.tod;
> +}
> +
>  /**
>   * get_clock_monotonic - returns current time in clock rate units
>   *
> @@ -216,7 +221,7 @@ static inline unsigned long get_tod_cloc
>  	unsigned long tod;
>  
>  	preempt_disable_notrace();
> -	tod = get_tod_clock() - tod_clock_base.tod;
> +	tod = __get_tod_clock_monotonic();
>  	preempt_enable_notrace();
>  	return tod;
>  }
...
> +unsigned long long noinstr sched_clock_noinstr(void)
> +{
> +	return tod_to_ns(__get_tod_clock_monotonic());
> +}
> +
>  /*
>   * Scheduler clock - returns current time in nanosec units.
>   */
>  unsigned long long notrace sched_clock(void)
>  {
> -	return tod_to_ns(get_tod_clock_monotonic());
> +	unsigned long long ns;
> +	preempt_disable_notrace();
> +	ns = tod_to_ns(get_tod_clock_monotonic());
> +	preempt_enable_notrace();
> +	return ns;
>  }
>  NOKPROBE_SYMBOL(sched_clock);

This disables preemption twice within sched_clock(). So this should either
call __get_tod_clock_monotonic() instead, or the function could stay as it
is, which I would prefer.

Re: [RFC][PATCH 6/9] s390/time: Provide sched_clock_noinstr()

[Peter Zijlstra]

On Tue, May 09, 2023 at 08:13:59AM +0200, Heiko Carstens wrote:
> 
> 1;115;0cOn Mon, May 08, 2023 at 11:19:57PM +0200, 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_noinstr()
> > function.
> > 
> > Specifically, preempt_enable_*() calls out to schedule(), which upsets
> > noinstr validation efforts.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/s390/include/asm/timex.h |   13 +++++++++----
> >  arch/s390/kernel/time.c       |   11 ++++++++++-
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> ...
> > +static __always_inline unsigned long __get_tod_clock_monotonic(void)
> > +{
> > +	return get_tod_clock() - tod_clock_base.tod;
> > +}
> > +
> >  /**
> >   * get_clock_monotonic - returns current time in clock rate units
> >   *
> > @@ -216,7 +221,7 @@ static inline unsigned long get_tod_cloc
> >  	unsigned long tod;
> >  
> >  	preempt_disable_notrace();
> > -	tod = get_tod_clock() - tod_clock_base.tod;
> > +	tod = __get_tod_clock_monotonic();
> >  	preempt_enable_notrace();
> >  	return tod;
> >  }
> ...
> > +unsigned long long noinstr sched_clock_noinstr(void)
> > +{
> > +	return tod_to_ns(__get_tod_clock_monotonic());
> > +}
> > +
> >  /*
> >   * Scheduler clock - returns current time in nanosec units.
> >   */
> >  unsigned long long notrace sched_clock(void)
> >  {
> > -	return tod_to_ns(get_tod_clock_monotonic());
> > +	unsigned long long ns;
> > +	preempt_disable_notrace();
> > +	ns = tod_to_ns(get_tod_clock_monotonic());
> > +	preempt_enable_notrace();
> > +	return ns;
> >  }
> >  NOKPROBE_SYMBOL(sched_clock);
> 
> This disables preemption twice within sched_clock(). So this should either
> call __get_tod_clock_monotonic() instead, or the function could stay as it
> is, which I would prefer.

Duh. Will fix.

[RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Peter Zijlstra]

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_noinstr()
function.

Specifically, preempt_enable_*() calls out to schedule(), which upsets
noinstr validation efforts.

  vmlinux.o: warning: objtool: native_sched_clock+0x96: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section
  vmlinux.o: warning: objtool: kvm_clock_read+0x22: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/kvmclock.c         |    4 +--
 arch/x86/kernel/tsc.c              |   38 ++++++++++++++++++++++++++++---------
 arch/x86/xen/time.c                |    3 --
 drivers/clocksource/hyperv_timer.c |    4 +--
 include/clocksource/hyperv_timer.h |    4 +--
 5 files changed, 36 insertions(+), 17 deletions(-)

--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -71,7 +71,7 @@ static int kvm_set_wallclock(const struc
 	return -ENODEV;
 }
 
-static noinstr u64 kvm_clock_read(void)
+static u64 kvm_clock_read(void)
 {
 	u64 ret;
 
@@ -88,7 +88,7 @@ static u64 kvm_clock_get_cycles(struct c
 
 static noinstr u64 kvm_sched_clock_read(void)
 {
-	return kvm_clock_read() - kvm_sched_clock_offset;
+	return pvclock_clocksource_read_nowd(this_cpu_pvti()) - kvm_sched_clock_offset;
 }
 
 static inline void kvm_sched_clock_init(bool stable)
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -69,12 +69,10 @@ static int __init tsc_early_khz_setup(ch
 }
 early_param("tsc_early_khz", tsc_early_khz_setup);
 
-__always_inline void cyc2ns_read_begin(struct cyc2ns_data *data)
+__always_inline void __cyc2ns_read(struct cyc2ns_data *data)
 {
 	int seq, idx;
 
-	preempt_disable_notrace();
-
 	do {
 		seq = this_cpu_read(cyc2ns.seq.seqcount.sequence);
 		idx = seq & 1;
@@ -86,6 +84,12 @@ __always_inline void cyc2ns_read_begin(s
 	} while (unlikely(seq != this_cpu_read(cyc2ns.seq.seqcount.sequence)));
 }
 
+__always_inline void cyc2ns_read_begin(struct cyc2ns_data *data)
+{
+	preempt_disable_notrace();
+	__cyc2ns_read(data);
+}
+
 __always_inline void cyc2ns_read_end(void)
 {
 	preempt_enable_notrace();
@@ -115,18 +119,25 @@ __always_inline void cyc2ns_read_end(voi
  *                      -johnstul@us.ibm.com "math is hard, lets go shopping!"
  */
 
-static __always_inline unsigned long long cycles_2_ns(unsigned long long cyc)
+static __always_inline unsigned long long __cycles_2_ns(unsigned long long cyc)
 {
 	struct cyc2ns_data data;
 	unsigned long long ns;
 
-	cyc2ns_read_begin(&data);
+	__cyc2ns_read(&data);
 
 	ns = data.cyc2ns_offset;
 	ns += mul_u64_u32_shr(cyc, data.cyc2ns_mul, data.cyc2ns_shift);
 
-	cyc2ns_read_end();
+	return ns;
+}
 
+static __always_inline unsigned long long cycles_2_ns(unsigned long long cyc)
+{
+	unsigned long long ns;
+	preempt_disable_notrace();
+	ns = __cycles_2_ns(cyc);
+	preempt_enable_notrace();
 	return ns;
 }
 
@@ -223,7 +234,7 @@ noinstr u64 native_sched_clock(void)
 		u64 tsc_now = rdtsc();
 
 		/* return the value in ns */
-		return cycles_2_ns(tsc_now);
+		return __cycles_2_ns(tsc_now);
 	}
 
 	/*
@@ -250,7 +261,7 @@ u64 native_sched_clock_from_tsc(u64 tsc)
 /* We need to define a real function for sched_clock, to override the
    weak default version */
 #ifdef CONFIG_PARAVIRT
-noinstr u64 sched_clock(void)
+noinstr u64 sched_clock_noinstr(void)
 {
 	return paravirt_sched_clock();
 }
@@ -260,11 +271,20 @@ bool using_native_sched_clock(void)
 	return static_call_query(pv_sched_clock) == native_sched_clock;
 }
 #else
-u64 sched_clock(void) __attribute__((alias("native_sched_clock")));
+u64 sched_clock_noinstr(void) __attribute__((alias("native_sched_clock")));
 
 bool using_native_sched_clock(void) { return true; }
 #endif
 
+notrace u64 sched_clock(void)
+{
+	u64 now;
+	preempt_disable_notrace();
+	now = sched_clock_noinstr();
+	preempt_enable_notrace();
+	return now;
+}
+
 int check_tsc_unstable(void)
 {
 	return tsc_unstable;
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -66,11 +66,10 @@ static noinstr u64 xen_sched_clock(void)
         struct pvclock_vcpu_time_info *src;
 	u64 ret;
 
-	preempt_disable_notrace();
 	src = &__this_cpu_read(xen_vcpu)->time;
 	ret = pvclock_clocksource_read_nowd(src);
 	ret -= xen_sched_clock_offset;
-	preempt_enable_notrace();
+
 	return ret;
 }
 
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
 	return read_hv_clock_tsc();
 }
 
-static u64 notrace read_hv_sched_clock_tsc(void)
+static u64 noinstr read_hv_sched_clock_tsc(void)
 {
-	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
+	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
 		(NSEC_PER_SEC / HV_CLOCK_HZ);
 }
 
--- a/include/clocksource/hyperv_timer.h
+++ b/include/clocksource/hyperv_timer.h
@@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
 extern unsigned long hv_get_tsc_pfn(void);
 extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
 
-static inline notrace u64
+static __always_inline notrace u64
 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
 {
 	u64 scale, offset;
@@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
 	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
 }
 
-static inline notrace u64
+static __always_inline notrace u64
 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
 {
 	u64 cur_tsc;

Re: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Peter Zijlstra]

On Mon, May 08, 2023 at 11:19:58PM +0200, Peter Zijlstra wrote:

> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
>  	return read_hv_clock_tsc();
>  }
>  
> -static u64 notrace read_hv_sched_clock_tsc(void)
> +static u64 noinstr read_hv_sched_clock_tsc(void)
>  {
> -	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
> +	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
>  		(NSEC_PER_SEC / HV_CLOCK_HZ);
>  }
>  
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
>  extern unsigned long hv_get_tsc_pfn(void);
>  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
>  
> -static inline notrace u64
> +static __always_inline notrace u64
>  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
>  {
>  	u64 scale, offset;
> @@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
>  	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
>  }
>  
> -static inline notrace u64
> +static __always_inline notrace u64
>  hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
>  {
>  	u64 cur_tsc;

Hyper-V folks!

While reviewing all this I found the following 'gem':

hv_init_clocksource()
  hv_setup_sched_clock()
    paravirt_set_sched_clock(read_hv_sched_clock_msr)

read_hv_sched_clock_msr() [notrace]
  read_hv_clock_msr()     [notrace]
    hv_get_register()      *traced*
      hv_get_non_nested_register() ...
        hv_ghcb_msr_read()
	  WARN_ON(in_nmi())
	  ...
	  local_irq_save()


Note that:

 a) sched_clock() is used in NMI context a *LOT*
 b) sched_clock() is notrace (or even noinstr with these patches)
    and local_irq_save() implies tracing


Can you pretty please:

 1) delete all this; or,
 2) fix it in a hurry?

Thanks!

Re: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Wei Liu]

On Mon, May 08, 2023 at 11:44:19PM +0200, Peter Zijlstra wrote:
> On Mon, May 08, 2023 at 11:19:58PM +0200, Peter Zijlstra wrote:
> 
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
> >  	return read_hv_clock_tsc();
> >  }
> >  
> > -static u64 notrace read_hv_sched_clock_tsc(void)
> > +static u64 noinstr read_hv_sched_clock_tsc(void)
> >  {
> > -	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
> > +	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
> >  		(NSEC_PER_SEC / HV_CLOCK_HZ);
> >  }
> >  
> > --- a/include/clocksource/hyperv_timer.h
> > +++ b/include/clocksource/hyperv_timer.h
> > @@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
> >  extern unsigned long hv_get_tsc_pfn(void);
> >  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> >  
> > -static inline notrace u64
> > +static __always_inline notrace u64
> >  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
> >  {
> >  	u64 scale, offset;
> > @@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
> >  	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
> >  }
> >  
> > -static inline notrace u64
> > +static __always_inline notrace u64
> >  hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> >  {
> >  	u64 cur_tsc;
> 
> Hyper-V folks!
> 
> While reviewing all this I found the following 'gem':
> 
> hv_init_clocksource()
>   hv_setup_sched_clock()
>     paravirt_set_sched_clock(read_hv_sched_clock_msr)
> 
> read_hv_sched_clock_msr() [notrace]
>   read_hv_clock_msr()     [notrace]
>     hv_get_register()      *traced*
>       hv_get_non_nested_register() ...
>         hv_ghcb_msr_read()
> 	  WARN_ON(in_nmi())
> 	  ...
> 	  local_irq_save()
> 
> 
> Note that:
> 
>  a) sched_clock() is used in NMI context a *LOT*
>  b) sched_clock() is notrace (or even noinstr with these patches)
>     and local_irq_save() implies tracing
> 

Tianyu and Michael, what's your thought on this?

Is the MSR-based GHCB usable at this point?

What other clock source can be used?

Thanks,
Wei.

> 
> Can you pretty please:
> 
>  1) delete all this; or,
>  2) fix it in a hurry?
> 
> Thanks!

Re: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Peter Zijlstra]

On Mon, May 08, 2023 at 11:30:43PM +0000, Wei Liu wrote:
> On Mon, May 08, 2023 at 11:44:19PM +0200, Peter Zijlstra wrote:
> > On Mon, May 08, 2023 at 11:19:58PM +0200, Peter Zijlstra wrote:
> > 
> > > --- a/drivers/clocksource/hyperv_timer.c
> > > +++ b/drivers/clocksource/hyperv_timer.c
> > > @@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
> > >  	return read_hv_clock_tsc();
> > >  }
> > >  
> > > -static u64 notrace read_hv_sched_clock_tsc(void)
> > > +static u64 noinstr read_hv_sched_clock_tsc(void)
> > >  {
> > > -	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
> > > +	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
> > >  		(NSEC_PER_SEC / HV_CLOCK_HZ);
> > >  }
> > >  
> > > --- a/include/clocksource/hyperv_timer.h
> > > +++ b/include/clocksource/hyperv_timer.h
> > > @@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
> > >  extern unsigned long hv_get_tsc_pfn(void);
> > >  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> > >  
> > > -static inline notrace u64
> > > +static __always_inline notrace u64
> > >  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
> > >  {
> > >  	u64 scale, offset;
> > > @@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
> > >  	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
> > >  }
> > >  
> > > -static inline notrace u64
> > > +static __always_inline notrace u64
> > >  hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> > >  {
> > >  	u64 cur_tsc;
> > 
> > Hyper-V folks!
> > 
> > While reviewing all this I found the following 'gem':
> > 
> > hv_init_clocksource()
> >   hv_setup_sched_clock()
> >     paravirt_set_sched_clock(read_hv_sched_clock_msr)
> > 
> > read_hv_sched_clock_msr() [notrace]
> >   read_hv_clock_msr()     [notrace]
> >     hv_get_register()      *traced*
> >       hv_get_non_nested_register() ...
> >         hv_ghcb_msr_read()
> > 	  WARN_ON(in_nmi())
> > 	  ...
> > 	  local_irq_save()
> > 
> > 
> > Note that:
> > 
> >  a) sched_clock() is used in NMI context a *LOT*
> >  b) sched_clock() is notrace (or even noinstr with these patches)
> >     and local_irq_save() implies tracing
> > 
> 
> Tianyu and Michael, what's your thought on this?
> 
> Is the MSR-based GHCB usable at this point?
> 
> What other clock source can be used?

You do have TSC support -- which is what I fixed for you. It's just the
whole MSR thing that is comically broken.

You could do a read_hv_clock_msr() implementation using
__rdmsr() and add some sanity checking that anything GHCB using (SEV?)
*will* use TSC.

Anyway, will you guys do that, or should I pull out the chainsaw and fix
it for you?

RE: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Michael Kelley (LINUX)]

From: Peter Zijlstra <peterz@infradead.org> Sent: Thursday, May 11, 2023 1:24 PM
> 
> On Mon, May 08, 2023 at 11:30:43PM +0000, Wei Liu wrote:
> > On Mon, May 08, 2023 at 11:44:19PM +0200, Peter Zijlstra wrote:
> > > On Mon, May 08, 2023 at 11:19:58PM +0200, Peter Zijlstra wrote:
> > >
> > > > --- a/drivers/clocksource/hyperv_timer.c
> > > > +++ b/drivers/clocksource/hyperv_timer.c
> > > > @@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
> > > >  	return read_hv_clock_tsc();
> > > >  }
> > > >
> > > > -static u64 notrace read_hv_sched_clock_tsc(void)
> > > > +static u64 noinstr read_hv_sched_clock_tsc(void)
> > > >  {
> > > > -	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
> > > > +	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
> > > >  		(NSEC_PER_SEC / HV_CLOCK_HZ);
> > > >  }
> > > >
> > > > --- a/include/clocksource/hyperv_timer.h
> > > > +++ b/include/clocksource/hyperv_timer.h
> > > > @@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
> > > >  extern unsigned long hv_get_tsc_pfn(void);
> > > >  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> > > >
> > > > -static inline notrace u64
> > > > +static __always_inline notrace u64
> > > >  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
> > > >  {
> > > >  	u64 scale, offset;
> > > > @@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
> > > >  	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
> > > >  }
> > > >
> > > > -static inline notrace u64
> > > > +static __always_inline notrace u64
> > > >  hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> > > >  {
> > > >  	u64 cur_tsc;
> > >
> > > Hyper-V folks!
> > >
> > > While reviewing all this I found the following 'gem':
> > >
> > > hv_init_clocksource()
> > >   hv_setup_sched_clock()
> > >     paravirt_set_sched_clock(read_hv_sched_clock_msr)
> > >
> > > read_hv_sched_clock_msr() [notrace]
> > >   read_hv_clock_msr()     [notrace]
> > >     hv_get_register()      *traced*
> > >       hv_get_non_nested_register() ...
> > >         hv_ghcb_msr_read()
> > > 	  WARN_ON(in_nmi())
> > > 	  ...
> > > 	  local_irq_save()
> > >
> > >
> > > Note that:
> > >
> > >  a) sched_clock() is used in NMI context a *LOT*
> > >  b) sched_clock() is notrace (or even noinstr with these patches)
> > >     and local_irq_save() implies tracing
> > >
> >
> > Tianyu and Michael, what's your thought on this?
> >
> > Is the MSR-based GHCB usable at this point?
> >
> > What other clock source can be used?
> 
> You do have TSC support -- which is what I fixed for you. It's just the
> whole MSR thing that is comically broken.
> 
> You could do a read_hv_clock_msr() implementation using
> __rdmsr() and add some sanity checking that anything GHCB using (SEV?)
> *will* use TSC.
> 
> Anyway, will you guys do that, or should I pull out the chainsaw and fix
> it for you?

Peter -- I'll work on a fix.  But it will be the first half of next week before
I can do it.

Michael

Re: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Peter Zijlstra]

On Thu, May 11, 2023 at 11:11:07PM +0000, Michael Kelley (LINUX) wrote:
> From: Peter Zijlstra <peterz@infradead.org> Sent: Thursday, May 11, 2023 1:24 PM

> > > Tianyu and Michael, what's your thought on this?
> > >
> > > Is the MSR-based GHCB usable at this point?
> > >
> > > What other clock source can be used?
> > 
> > You do have TSC support -- which is what I fixed for you. It's just the
> > whole MSR thing that is comically broken.
> > 
> > You could do a read_hv_clock_msr() implementation using
> > __rdmsr() and add some sanity checking that anything GHCB using (SEV?)
> > *will* use TSC.
> > 
> > Anyway, will you guys do that, or should I pull out the chainsaw and fix
> > it for you?
> 
> Peter -- I'll work on a fix.  But it will be the first half of next week before
> I can do it.

OK, Thanks!

RE: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Michael Kelley (LINUX)]

From: Peter Zijlstra <peterz@infradead.org> Sent: Monday, May 8, 2023 2:44 PM
> 
> On Mon, May 08, 2023 at 11:19:58PM +0200, Peter Zijlstra wrote:
> 
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -408,9 +408,9 @@ static u64 notrace read_hv_clock_tsc_cs(
> >  	return read_hv_clock_tsc();
> >  }
> >
> > -static u64 notrace read_hv_sched_clock_tsc(void)
> > +static u64 noinstr read_hv_sched_clock_tsc(void)
> >  {
> > -	return (read_hv_clock_tsc() - hv_sched_clock_offset) *
> > +	return (hv_read_tsc_page(hv_get_tsc_page()) - hv_sched_clock_offset) *
> >  		(NSEC_PER_SEC / HV_CLOCK_HZ);
> >  }
> >
> > --- a/include/clocksource/hyperv_timer.h
> > +++ b/include/clocksource/hyperv_timer.h
> > @@ -38,7 +38,7 @@ extern void hv_remap_tsc_clocksource(voi
> >  extern unsigned long hv_get_tsc_pfn(void);
> >  extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
> >
> > -static inline notrace u64
> > +static __always_inline notrace u64
> >  hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg, u64 *cur_tsc)
> >  {
> >  	u64 scale, offset;
> > @@ -85,7 +85,7 @@ hv_read_tsc_page_tsc(const struct ms_hyp
> >  	return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
> >  }
> >
> > -static inline notrace u64
> > +static __always_inline notrace u64
> >  hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
> >  {
> >  	u64 cur_tsc;
> 
> Hyper-V folks!
> 
> While reviewing all this I found the following 'gem':
> 
> hv_init_clocksource()
>   hv_setup_sched_clock()
>     paravirt_set_sched_clock(read_hv_sched_clock_msr)
> 
> read_hv_sched_clock_msr() [notrace]
>   read_hv_clock_msr()     [notrace]
>     hv_get_register()      *traced*
>       hv_get_non_nested_register() ...
>         hv_ghcb_msr_read()
> 	  WARN_ON(in_nmi())
> 	  ...
> 	  local_irq_save()
> 
> 
> Note that:
> 
>  a) sched_clock() is used in NMI context a *LOT*
>  b) sched_clock() is notrace (or even noinstr with these patches)
>     and local_irq_save() implies tracing
> 
> 
> Can you pretty please:
> 
>  1) delete all this; or,
>  2) fix it in a hurry?
> 

Peter -- I've sent you an RFC patch to incorporate into your broader
patch set.  I think it probably makes sense for all the Hyper-V
stuff to be a separate patch.

I haven't previously worked with the details of notrace vs. noinstr,
but I followed the patterns elsewhere in patch set. Please review
to see if it seems correct.

One thing:  In the cases where I added __always_inline, I dropped
any notrace or noinstr annotations.  I presume such code always
takes on the attributes of the caller.  If that's not correct, let me know.

Michael

Re: [RFC][PATCH 7/9] x86/tsc: Provide sched_clock_noinstr()

[Peter Zijlstra]

On Wed, May 17, 2023 at 02:26:35AM +0000, Michael Kelley (LINUX) wrote:

> Peter -- I've sent you an RFC patch to incorporate into your broader
> patch set.  I think it probably makes sense for all the Hyper-V
> stuff to be a separate patch.

Perhaps, it's not that much.

> I haven't previously worked with the details of notrace vs. noinstr,
> but I followed the patterns elsewhere in patch set. Please review
> to see if it seems correct.

notrace inhibits the "call __fentry__" at the start of the symbol.

The __fentry__ call is mostly for ftrace, there's a few sites where
inhibiting tracing is critical -- stuff that happens before the ftrace
recursion handling, but mostly it's about performance these days,
constantly hitting the recusion code isn't very good.

noinstr inhibits any and all compiler generated 'extra' -- it is for the
C as a portable assembler usage. This very much includes notrace, but it
also covers all the *SAN nonsense. Basically, if it does not directly
reflect the code as written, it shouldn't be emitted.

Additionally, and for validation purposes, it also ensures all these
symbols end up in a special text section.

But yeah, you seem to have gotten it right.

> One thing:  In the cases where I added __always_inline, I dropped
> any notrace or noinstr annotations.  I presume such code always
> takes on the attributes of the caller.  If that's not correct, let me know.

Correct; noinstr actually has an explicit noinline because compilers
suck :/

[RFC][PATCH 8/9] sched/clock: Provide local_clock_noinstr()

[Peter Zijlstra]

Now that all ARCH_WANTS_NO_INSTR architectures (arm64, loongarch,
s390, x86) provide sched_clock_noinstr(), use this to provide
local_clock_noinstr().

This local_clock_noinstr() will be safe to use from noinstr code with
the assumption that any such noinstr code is non-preemptible (it had
better be, entry code will have IRQs disabled while __cpuidle must
have preemption disabled).

Specifically, preempt_enable_notrace(), a common part of many a
sched_clock() implementation calls out to schedule() -- even though,
per the above, it will never trigger -- which frustrates noinstr
validation.

  vmlinux.o: warning: objtool: local_clock+0xb5: call to preempt_schedule_notrace_thunk() leaves .noinstr.text section

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched/clock.h |   17 ++++++++++++++++-
 kernel/sched/clock.c        |   19 +++++++++++++------
 2 files changed, 29 insertions(+), 7 deletions(-)

--- a/include/linux/sched/clock.h
+++ b/include/linux/sched/clock.h
@@ -12,7 +12,16 @@
  *
  * Please use one of the three interfaces below.
  */
-extern unsigned long long notrace sched_clock(void);
+extern u64 sched_clock(void);
+
+#if defined(CONFIG_ARCH_WANTS_NO_INSTR) || defined(CONFIG_GENERIC_SCHED_CLOCK)
+extern u64 sched_clock_noinstr(void);
+#else
+__always_inline u64 sched_clock_noinstr(void)
+{
+	return sched_clock();
+}
+#endif
 
 /*
  * See the comment in kernel/sched/clock.c
@@ -45,6 +54,11 @@ static inline u64 cpu_clock(int cpu)
 	return sched_clock();
 }
 
+static __always_inline u64 local_clock_noinstr(void)
+{
+	return sched_clock_noinstr();
+}
+
 static __always_inline u64 local_clock(void)
 {
 	return sched_clock();
@@ -79,6 +93,7 @@ static inline u64 cpu_clock(int cpu)
 	return sched_clock_cpu(cpu);
 }
 
+extern u64 local_clock_noinstr(void);
 extern u64 local_clock(void);
 
 #endif
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -266,7 +266,7 @@ static __always_inline u64 sched_clock_l
 	s64 delta;
 
 again:
-	now = sched_clock();
+	now = sched_clock_noinstr();
 	delta = now - scd->tick_raw;
 	if (unlikely(delta < 0))
 		delta = 0;
@@ -293,22 +293,29 @@ static __always_inline u64 sched_clock_l
 	return clock;
 }
 
-noinstr u64 local_clock(void)
+noinstr u64 local_clock_noinstr(void)
 {
 	u64 clock;
 
 	if (static_branch_likely(&__sched_clock_stable))
-		return sched_clock() + __sched_clock_offset;
+		return sched_clock_noinstr() + __sched_clock_offset;
 
 	if (!static_branch_likely(&sched_clock_running))
-		return sched_clock();
+		return sched_clock_noinstr();
 
-	preempt_disable_notrace();
 	clock = sched_clock_local(this_scd());
-	preempt_enable_notrace();
 
 	return clock;
 }
+
+u64 local_clock(void)
+{
+	u64 now;
+	preempt_disable_notrace();
+	now = local_clock_noinstr();
+	preempt_enable_notrace();
+	return now;
+}
 EXPORT_SYMBOL_GPL(local_clock);
 
 static notrace u64 sched_clock_remote(struct sched_clock_data *scd)

[RFC][PATCH 9/9] cpuidle: Use local_clock_noinstr()

[Peter Zijlstra]

With the introduction of local_clock_noinstr(), local_clock() itself
is no longer marked noinstr, use the correct function.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/cpuidle/cpuidle.c    |    8 ++++----
 drivers/cpuidle/poll_state.c |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,7 +145,7 @@ static noinstr void enter_s2idle_proper(
 
 	instrumentation_begin();
 
-	time_start = ns_to_ktime(local_clock());
+	time_start = ns_to_ktime(local_clock_noinstr());
 
 	tick_freeze();
 	/*
@@ -169,7 +169,7 @@ static noinstr void enter_s2idle_proper(
 	tick_unfreeze();
 	start_critical_timings();
 
-	time_end = ns_to_ktime(local_clock());
+	time_end = ns_to_ktime(local_clock_noinstr());
 
 	dev->states_usage[index].s2idle_time += ktime_us_delta(time_end, time_start);
 	dev->states_usage[index].s2idle_usage++;
@@ -243,7 +243,7 @@ noinstr int cpuidle_enter_state(struct c
 	sched_idle_set_state(target_state);
 
 	trace_cpu_idle(index, dev->cpu);
-	time_start = ns_to_ktime(local_clock());
+	time_start = ns_to_ktime(local_clock_noinstr());
 
 	stop_critical_timings();
 	if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE)) {
@@ -276,7 +276,7 @@ noinstr int cpuidle_enter_state(struct c
 	start_critical_timings();
 
 	sched_clock_idle_wakeup_event();
-	time_end = ns_to_ktime(local_clock());
+	time_end = ns_to_ktime(local_clock_noinstr());
 	trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
 
 	/* The cpu is no longer idle or about to enter idle. */
--- a/drivers/cpuidle/poll_state.c
+++ b/drivers/cpuidle/poll_state.c
@@ -15,7 +15,7 @@ static int __cpuidle poll_idle(struct cp
 {
 	u64 time_start;
 
-	time_start = local_clock();
+	time_start = local_clock_noinstr();
 
 	dev->poll_time_limit = false;
 
@@ -32,7 +32,7 @@ static int __cpuidle poll_idle(struct cp
 				continue;
 
 			loop_count = 0;
-			if (local_clock() - time_start > limit) {
+			if (local_clock_noinstr() - time_start > limit) {
 				dev->poll_time_limit = true;
 				break;
 			}

Re: [RFC][PATCH 9/9] cpuidle: Use local_clock_noinstr()

[Rafael J. Wysocki]

On Mon, May 8, 2023 at 11:34 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> With the introduction of local_clock_noinstr(), local_clock() itself
> is no longer marked noinstr, use the correct function.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/cpuidle/cpuidle.c    |    8 ++++----
>  drivers/cpuidle/poll_state.c |    4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -145,7 +145,7 @@ static noinstr void enter_s2idle_proper(
>
>         instrumentation_begin();
>
> -       time_start = ns_to_ktime(local_clock());
> +       time_start = ns_to_ktime(local_clock_noinstr());
>
>         tick_freeze();
>         /*
> @@ -169,7 +169,7 @@ static noinstr void enter_s2idle_proper(
>         tick_unfreeze();
>         start_critical_timings();
>
> -       time_end = ns_to_ktime(local_clock());
> +       time_end = ns_to_ktime(local_clock_noinstr());
>
>         dev->states_usage[index].s2idle_time += ktime_us_delta(time_end, time_start);
>         dev->states_usage[index].s2idle_usage++;
> @@ -243,7 +243,7 @@ noinstr int cpuidle_enter_state(struct c
>         sched_idle_set_state(target_state);
>
>         trace_cpu_idle(index, dev->cpu);
> -       time_start = ns_to_ktime(local_clock());
> +       time_start = ns_to_ktime(local_clock_noinstr());
>
>         stop_critical_timings();
>         if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE)) {
> @@ -276,7 +276,7 @@ noinstr int cpuidle_enter_state(struct c
>         start_critical_timings();
>
>         sched_clock_idle_wakeup_event();
> -       time_end = ns_to_ktime(local_clock());
> +       time_end = ns_to_ktime(local_clock_noinstr());
>         trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
>
>         /* The cpu is no longer idle or about to enter idle. */
> --- a/drivers/cpuidle/poll_state.c
> +++ b/drivers/cpuidle/poll_state.c
> @@ -15,7 +15,7 @@ static int __cpuidle poll_idle(struct cp
>  {
>         u64 time_start;
>
> -       time_start = local_clock();
> +       time_start = local_clock_noinstr();
>
>         dev->poll_time_limit = false;
>
> @@ -32,7 +32,7 @@ static int __cpuidle poll_idle(struct cp
>                                 continue;
>
>                         loop_count = 0;
> -                       if (local_clock() - time_start > limit) {
> +                       if (local_clock_noinstr() - time_start > limit) {
>                                 dev->poll_time_limit = true;
>                                 break;
>                         }
>

The above LGTM, but the teo governors uses local_clock() too.  Should
it use the _noinstr() version?

Re: [RFC][PATCH 9/9] cpuidle: Use local_clock_noinstr()

[Peter Zijlstra]

On Tue, May 09, 2023 at 06:18:08PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 8, 2023 at 11:34 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > --- a/drivers/cpuidle/poll_state.c
> > +++ b/drivers/cpuidle/poll_state.c
> > @@ -15,7 +15,7 @@ static int __cpuidle poll_idle(struct cp
> >  {
> >         u64 time_start;
> >
> > -       time_start = local_clock();
> > +       time_start = local_clock_noinstr();
> >
> >         dev->poll_time_limit = false;
> >
> > @@ -32,7 +32,7 @@ static int __cpuidle poll_idle(struct cp
> >                                 continue;
> >
> >                         loop_count = 0;
> > -                       if (local_clock() - time_start > limit) {
> > +                       if (local_clock_noinstr() - time_start > limit) {
> >                                 dev->poll_time_limit = true;
> >                                 break;
> >                         }
> >
> 
> The above LGTM, but the teo governors uses local_clock() too.  Should
> it use the _noinstr() version?

Only the callsites from noinstr or __cpuidle functions, IIRC the
governors are neither and should be OK.

Re: [RFC][PATCH 0/9] local_clock() vs noinstr

[Sebastian Andrzej Siewior]

On 2023-05-08 23:19:51 [+0200], Peter Zijlstra wrote:
> Hi all!
Hi Peter,

> Compile tested only on x86_64/s390/arm64 -- I've just fed it to the
Thanks, this works.

Sebastian