[GIT PULL] Generic entry for ARM

[GIT PULL] Generic entry for ARM

[Linus Walleij]

Hi Russell,

please consider pulling the following git branch for generic entry,
see below.

This branch was just harvested from my own v5 patch series on
lore with b4 am -t 20250225-arm-generic-entry-v5-0-2f02313653e5@linaro.org
then git am on top of v6.14-rc1, so you can do the same if you
prefer.

It's possible to squash patches, even all of them into one
big all-or-nothing patch, given the not very gradual nature generic
entry conversion seems to have.

Main upsides and downsides are in the signed tag.

I don't know who the most important stakeholders are, but I guess
the context tracker maintainer (Paul McKenney) and the people working
on generic entry (tglx) could have a say on how important this is, or isn't.

I think it's pretty neat.

Yours,
Linus Walleij

The following changes since commit 2014c95afecee3e76ca4a56956a936e23283f05b:

  Linux 6.14-rc1 (2025-02-02 15:39:26 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git
tags/arm-generic-entry-for-v6.15

for you to fetch changes up to 98b8e133458a3feefdf882ea16fb7f1f576a49e5:

  ARM: entry: Reimplement local restart in C (2025-02-28 13:40:44 +0100)

----------------------------------------------------------------
Main upsides:

- Using the same common entry as used by x86_64, RISCV, S390
  and Loongarch, probably soon also ARM64.

- Moves ARM away from the obsoleted context tracker entry points
  user_enter_callable() and user_exit_callable() are now only used
  by ARM, CSKY and Xtensa.

- Solves a few lockdep warnings in the process.

- Converts a bit of assembly into C.

Main downside:

- Slightly increased system call overhead, around 6% in
  measurements.

----------------------------------------------------------------
Linus Walleij (31):
      ARM: Prepare includes for generic entry
      ARM: ptrace: Split report_syscall()
      ARM: entry: Skip ret_slow_syscall label
      ARM: process: Rewrite ret_from_fork i C
      ARM: process: Remove local restart
      ARM: entry: Invoke syscalls using C
      ARM: entry: Rewrite two asm calls in C
      ARM: entry: Move trace entry to C function
      ARM: entry: save the syscall sp in thread_info
      ARM: entry: move all tracing invocation to C
      ARM: entry: Merge the common and trace entry code
      ARM: entry: Rename syscall invocation
      ARM: entry: Create user_mode_enter/exit
      ARM: entry: Drop trace argument from usr_entry macro
      ARM: entry: Separate call path for syscall SWI entry
      ARM: entry: Drop argument to asm_irqentry macros
      ARM: entry: Implement syscall_exit_to_user_mode()
      ARM: entry: Drop the superfast ret_fast_syscall
      ARM: entry: Remove fast and offset register restore
      ARM: entry: Untangle ret_fast_syscall/to_user
      ARM: entry: Do not double-call exit functions
      ARM: entry: Move work processing to C
      ARM: entry: Stop exiting syscalls like IRQs
      ARM: entry: Complete syscall and IRQ transition to C
      ARM: entry: Create irqentry calls from kernel mode
      ARM: entry: Move in-kernel hardirq tracing to C
      ARM: irq: Add irqstack helper
      ARM: entry: Convert to generic entry
      ARM: entry: Handle dabt, pabt, and und as interrupts
      ARM: entry: Block IRQs in early IRQ context
      ARM: entry: Reimplement local restart in C

 arch/arm/Kconfig                    |   1 +
 arch/arm/include/asm/entry-common.h |  66 ++++++++++++
 arch/arm/include/asm/entry.h        |  14 +++
 arch/arm/include/asm/ptrace.h       |   8 +-
 arch/arm/include/asm/signal.h       |   4 -
 arch/arm/include/asm/stacktrace.h   |   2 +-
 arch/arm/include/asm/switch_to.h    |   4 +
 arch/arm/include/asm/syscall.h      |   7 ++
 arch/arm/include/asm/thread_info.h  |  22 ++--
 arch/arm/include/asm/traps.h        |   5 +-
 arch/arm/include/uapi/asm/ptrace.h  |   2 +
 arch/arm/kernel/Makefile            |   5 +-
 arch/arm/kernel/asm-offsets.c       |   1 +
 arch/arm/kernel/entry-armv.S        |  82 ++++-----------
 arch/arm/kernel/entry-common.S      | 198 +++++++++++++-----------------------
 arch/arm/kernel/entry-header.S      | 100 ++----------------
 arch/arm/kernel/entry.c             | 120 ++++++++++++++++++++++
 arch/arm/kernel/irq.c               |   6 ++
 arch/arm/kernel/irq.h               |   2 +
 arch/arm/kernel/process.c           |  25 ++++-
 arch/arm/kernel/ptrace.c            |  81 +--------------
 arch/arm/kernel/signal.c            |  68 +++----------
 arch/arm/kernel/syscall.c           |  59 +++++++++++
 arch/arm/kernel/traps.c             |  30 +-----
 arch/arm/mm/abort-ev4.S             |   2 +-
 arch/arm/mm/abort-ev4t.S            |   2 +-
 arch/arm/mm/abort-ev5t.S            |   4 +-
 arch/arm/mm/abort-ev5tj.S           |   6 +-
 arch/arm/mm/abort-ev6.S             |   2 +-
 arch/arm/mm/abort-ev7.S             |   2 +-
 arch/arm/mm/abort-lv4t.S            |  36 +++----
 arch/arm/mm/abort-macro.S           |   2 +-
 arch/arm/mm/abort-nommu.S           |   2 +-
 arch/arm/mm/fault.c                 |   4 +-
 arch/arm/mm/fault.h                 |   8 +-
 arch/arm/mm/pabort-legacy.S         |   2 +-
 arch/arm/mm/pabort-v6.S             |   2 +-
 arch/arm/mm/pabort-v7.S             |   2 +-
 38 files changed, 484 insertions(+), 504 deletions(-)
 create mode 100644 arch/arm/include/asm/entry-common.h
 create mode 100644 arch/arm/include/asm/entry.h
 create mode 100644 arch/arm/kernel/entry.c
 create mode 100644 arch/arm/kernel/irq.h
 create mode 100644 arch/arm/kernel/syscall.c

[GIT PULL] Generic entry for ARM

[phil995511]

It is a very bad idea to want to code the Linux kernel for ARM in C++ !!!

Our ARM CPUs on RPi and others are not powerful enough to use a Linux kernel written in C++ and therefore lose performance, while these ARM CPUs are already extremely low-performance !!!

Suggesting to code like this is to be in favor of planned obsolescence, of the scrapping of thousands of small machines ;-(

Regards.

https://lore.kernel.org/lkml/CACRpkdZCiiMTwf7eGJJ9aCKFOC3_xTGv1JKQUijjyp+_++cZ_A@mail.gmail.com/

Envoyé avec la messagerie sécurisée Proton Mail.

Re: [GIT PULL] Generic entry for ARM

[Russell King (Oracle)]

On Sun, Mar 02, 2025 at 03:23:20PM +0000, phil995511 wrote:
> It is a very bad idea to want to code the Linux kernel for ARM in C++ !!!
> 
> Our ARM CPUs on RPi and others are not powerful enough to use a Linux kernel written in C++ and therefore lose performance, while these ARM CPUs are already extremely low-performance !!!
> 
> Suggesting to code like this is to be in favor of planned obsolescence, of the scrapping of thousands of small machines ;-(

With all due respect, you have your knickers in a twist.

This is not about C++, it's about plain old fashioned C, which has been
the preferred kernel programming language for decades. I have no idea
where you've got the idea that LinusW's commit has anything to do with
C++.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [GIT PULL] Generic entry for ARM

[Thomas Gleixner]

On Fri, Feb 28 2025 at 13:49, Linus Walleij wrote:
> please consider pulling the following git branch for generic entry,
> see below.
>
> This branch was just harvested from my own v5 patch series on
> lore with b4 am -t 20250225-arm-generic-entry-v5-0-2f02313653e5@linaro.org
> then git am on top of v6.14-rc1, so you can do the same if you
> prefer.
>
> It's possible to squash patches, even all of them into one
> big all-or-nothing patch, given the not very gradual nature generic
> entry conversion seems to have.
>
> Main upsides and downsides are in the signed tag.
>
> I don't know who the most important stakeholders are, but I guess
> the context tracker maintainer (Paul McKenney) and the people working
> on generic entry (tglx) could have a say on how important this is, or isn't.
>
> I think it's pretty neat.

It's valuable to consolidate these common functionalities across
architectures as it reduces maintainence costs.

I read through the patches and did not find anything offending there.

Thanks for doing that!

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

Re: [GIT PULL] Generic entry for ARM

[Paul E. McKenney]

On Fri, Feb 28, 2025 at 01:49:36PM +0100, Linus Walleij wrote:
> Hi Russell,
> 
> please consider pulling the following git branch for generic entry,
> see below.
> 
> This branch was just harvested from my own v5 patch series on
> lore with b4 am -t 20250225-arm-generic-entry-v5-0-2f02313653e5@linaro.org
> then git am on top of v6.14-rc1, so you can do the same if you
> prefer.
> 
> It's possible to squash patches, even all of them into one
> big all-or-nothing patch, given the not very gradual nature generic
> entry conversion seems to have.
> 
> Main upsides and downsides are in the signed tag.
> 
> I don't know who the most important stakeholders are, but I guess
> the context tracker maintainer (Paul McKenney) and the people working
> on generic entry (tglx) could have a say on how important this is, or isn't.
> 
> I think it's pretty neat.

No argument here!

Once you are confident that you have all the needed "noinstr"
and "__always_inline" instances in place, could you please add
ARCH_WANTS_NO_INSTR to the list of "select" clauses for "config ARM"
in arch/arm/Kconfig?

Once all architectures that support trampoline-based tracing also select
ARCH_WANTS_NO_INSTR we can drop RCU Tasks Rude.  ;-)

In the meantime, for the series:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

							Thanx, Paul

> Yours,
> Linus Walleij
> 
> The following changes since commit 2014c95afecee3e76ca4a56956a936e23283f05b:
> 
>   Linux 6.14-rc1 (2025-02-02 15:39:26 -0800)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git
> tags/arm-generic-entry-for-v6.15
> 
> for you to fetch changes up to 98b8e133458a3feefdf882ea16fb7f1f576a49e5:
> 
>   ARM: entry: Reimplement local restart in C (2025-02-28 13:40:44 +0100)
> 
> ----------------------------------------------------------------
> Main upsides:
> 
> - Using the same common entry as used by x86_64, RISCV, S390
>   and Loongarch, probably soon also ARM64.
> 
> - Moves ARM away from the obsoleted context tracker entry points
>   user_enter_callable() and user_exit_callable() are now only used
>   by ARM, CSKY and Xtensa.
> 
> - Solves a few lockdep warnings in the process.
> 
> - Converts a bit of assembly into C.
> 
> Main downside:
> 
> - Slightly increased system call overhead, around 6% in
>   measurements.
> 
> ----------------------------------------------------------------
> Linus Walleij (31):
>       ARM: Prepare includes for generic entry
>       ARM: ptrace: Split report_syscall()
>       ARM: entry: Skip ret_slow_syscall label
>       ARM: process: Rewrite ret_from_fork i C
>       ARM: process: Remove local restart
>       ARM: entry: Invoke syscalls using C
>       ARM: entry: Rewrite two asm calls in C
>       ARM: entry: Move trace entry to C function
>       ARM: entry: save the syscall sp in thread_info
>       ARM: entry: move all tracing invocation to C
>       ARM: entry: Merge the common and trace entry code
>       ARM: entry: Rename syscall invocation
>       ARM: entry: Create user_mode_enter/exit
>       ARM: entry: Drop trace argument from usr_entry macro
>       ARM: entry: Separate call path for syscall SWI entry
>       ARM: entry: Drop argument to asm_irqentry macros
>       ARM: entry: Implement syscall_exit_to_user_mode()
>       ARM: entry: Drop the superfast ret_fast_syscall
>       ARM: entry: Remove fast and offset register restore
>       ARM: entry: Untangle ret_fast_syscall/to_user
>       ARM: entry: Do not double-call exit functions
>       ARM: entry: Move work processing to C
>       ARM: entry: Stop exiting syscalls like IRQs
>       ARM: entry: Complete syscall and IRQ transition to C
>       ARM: entry: Create irqentry calls from kernel mode
>       ARM: entry: Move in-kernel hardirq tracing to C
>       ARM: irq: Add irqstack helper
>       ARM: entry: Convert to generic entry
>       ARM: entry: Handle dabt, pabt, and und as interrupts
>       ARM: entry: Block IRQs in early IRQ context
>       ARM: entry: Reimplement local restart in C
> 
>  arch/arm/Kconfig                    |   1 +
>  arch/arm/include/asm/entry-common.h |  66 ++++++++++++
>  arch/arm/include/asm/entry.h        |  14 +++
>  arch/arm/include/asm/ptrace.h       |   8 +-
>  arch/arm/include/asm/signal.h       |   4 -
>  arch/arm/include/asm/stacktrace.h   |   2 +-
>  arch/arm/include/asm/switch_to.h    |   4 +
>  arch/arm/include/asm/syscall.h      |   7 ++
>  arch/arm/include/asm/thread_info.h  |  22 ++--
>  arch/arm/include/asm/traps.h        |   5 +-
>  arch/arm/include/uapi/asm/ptrace.h  |   2 +
>  arch/arm/kernel/Makefile            |   5 +-
>  arch/arm/kernel/asm-offsets.c       |   1 +
>  arch/arm/kernel/entry-armv.S        |  82 ++++-----------
>  arch/arm/kernel/entry-common.S      | 198 +++++++++++++-----------------------
>  arch/arm/kernel/entry-header.S      | 100 ++----------------
>  arch/arm/kernel/entry.c             | 120 ++++++++++++++++++++++
>  arch/arm/kernel/irq.c               |   6 ++
>  arch/arm/kernel/irq.h               |   2 +
>  arch/arm/kernel/process.c           |  25 ++++-
>  arch/arm/kernel/ptrace.c            |  81 +--------------
>  arch/arm/kernel/signal.c            |  68 +++----------
>  arch/arm/kernel/syscall.c           |  59 +++++++++++
>  arch/arm/kernel/traps.c             |  30 +-----
>  arch/arm/mm/abort-ev4.S             |   2 +-
>  arch/arm/mm/abort-ev4t.S            |   2 +-
>  arch/arm/mm/abort-ev5t.S            |   4 +-
>  arch/arm/mm/abort-ev5tj.S           |   6 +-
>  arch/arm/mm/abort-ev6.S             |   2 +-
>  arch/arm/mm/abort-ev7.S             |   2 +-
>  arch/arm/mm/abort-lv4t.S            |  36 +++----
>  arch/arm/mm/abort-macro.S           |   2 +-
>  arch/arm/mm/abort-nommu.S           |   2 +-
>  arch/arm/mm/fault.c                 |   4 +-
>  arch/arm/mm/fault.h                 |   8 +-
>  arch/arm/mm/pabort-legacy.S         |   2 +-
>  arch/arm/mm/pabort-v6.S             |   2 +-
>  arch/arm/mm/pabort-v7.S             |   2 +-
>  38 files changed, 484 insertions(+), 504 deletions(-)
>  create mode 100644 arch/arm/include/asm/entry-common.h
>  create mode 100644 arch/arm/include/asm/entry.h
>  create mode 100644 arch/arm/kernel/entry.c
>  create mode 100644 arch/arm/kernel/irq.h
>  create mode 100644 arch/arm/kernel/syscall.c

Re: [GIT PULL] Generic entry for ARM

[Linus Walleij]

On Wed, Mar 12, 2025 at 7:00 PM Paul E. McKenney <paulmck@kernel.org> wrote:

> Once you are confident that you have all the needed "noinstr"
> and "__always_inline" instances in place, could you please add
> ARCH_WANTS_NO_INSTR to the list of "select" clauses for "config ARM"
> in arch/arm/Kconfig?

I would love to do that, I'm just not sure what this really entails.

Surely this patchset tags a noinstr on every entry point from
exceptions and syscall software interrupts.
Documentation/core-api/entry.rst is pretty good at explaining this.

But what makes me uncertain are things that are tagged
"notrace", such as void notrace cpu_init(void) - surely we
don't trace, but should that be "noinstr"? It's even marked
"notrace" but not "noinstr" in arm64.

cpu_init() is called from e.g.:
asmlinkage void secondary_start_kernel(struct task_struct *task)
OK should this also be noinstr? Or is that just implied because
of asmlinkage?

<linux/compiler_types.h> will resolve to:

#if defined(CC_USING_HOTPATCH)
#define notrace                 __attribute__((hotpatch(0, 0)))
#elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
#define notrace                 __attribute__((patchable_function_entry(0, 0)))
#else
#define notrace                 __attribute__((__no_instrument_function__))
#endif

which I read as three different ways of saying "don't patch here".

Which is confusingly similar or identical to what noinstr does, I do see that
noinstr pushes the code to separate section but that in turn might
be what __attribute__((__no_instrument_function__)) and
friends does?

Are they equivalent?

sched_clock_noinstr() is tagged noinstr and sched_clock() is
tagged notrace, so there must be a difference here.

secondary_start_kernel() is tagged "notrace" on arm64 but
not on arm, should it be tagged "noinstr" or "notrace"?

This kind of stuff makes me uncertain about how this is to be
done. A "noinstr vs notrace" section in Documentation/core-api/entry.rst
would help a lot I think!

Yours,
Linus Walleij

Re: [GIT PULL] Generic entry for ARM

[Paul E. McKenney]

On Mon, Mar 31, 2025 at 11:48:40PM +0200, Linus Walleij wrote:
> On Wed, Mar 12, 2025 at 7:00 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> > Once you are confident that you have all the needed "noinstr"
> > and "__always_inline" instances in place, could you please add
> > ARCH_WANTS_NO_INSTR to the list of "select" clauses for "config ARM"
> > in arch/arm/Kconfig?
> 
> I would love to do that, I'm just not sure what this really entails.

I freely confess that I have only seen this done, not done it myself.

> Surely this patchset tags a noinstr on every entry point from
> exceptions and syscall software interrupts.
> Documentation/core-api/entry.rst is pretty good at explaining this.
> 
> But what makes me uncertain are things that are tagged
> "notrace", such as void notrace cpu_init(void) - surely we
> don't trace, but should that be "noinstr"? It's even marked
> "notrace" but not "noinstr" in arm64.

I know that __always_inline does the trick, but I am not sure about
notrace (my guess is "no" on notrace).  I added the objtool maintainers
on CC.

No argument on the usefulness of documentation.  ;-)

							Thanx, Paul

> cpu_init() is called from e.g.:
> asmlinkage void secondary_start_kernel(struct task_struct *task)
> OK should this also be noinstr? Or is that just implied because
> of asmlinkage?
> 
> <linux/compiler_types.h> will resolve to:
> 
> #if defined(CC_USING_HOTPATCH)
> #define notrace                 __attribute__((hotpatch(0, 0)))
> #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
> #define notrace                 __attribute__((patchable_function_entry(0, 0)))
> #else
> #define notrace                 __attribute__((__no_instrument_function__))
> #endif
> 
> which I read as three different ways of saying "don't patch here".
> 
> Which is confusingly similar or identical to what noinstr does, I do see that
> noinstr pushes the code to separate section but that in turn might
> be what __attribute__((__no_instrument_function__)) and
> friends does?
> 
> Are they equivalent?
> 
> sched_clock_noinstr() is tagged noinstr and sched_clock() is
> tagged notrace, so there must be a difference here.
> 
> secondary_start_kernel() is tagged "notrace" on arm64 but
> not on arm, should it be tagged "noinstr" or "notrace"?
> 
> This kind of stuff makes me uncertain about how this is to be
> done. A "noinstr vs notrace" section in Documentation/core-api/entry.rst
> would help a lot I think!
> 
> Yours,
> Linus Walleij

Re: [GIT PULL] Generic entry for ARM

[Josh Poimboeuf]

I think Thomas, Peter and Steven are the experts here, but I'll give my
limited understanding.

On Mon, Apr 07, 2025 at 10:00:54AM -0700, Paul E. McKenney wrote:
> > cpu_init() is called from e.g.:
> > asmlinkage void secondary_start_kernel(struct task_struct *task)
> > OK should this also be noinstr? Or is that just implied because
> > of asmlinkage?
> >
> > <linux/compiler_types.h> will resolve to:
> > 
> > #if defined(CC_USING_HOTPATCH)
> > #define notrace                 __attribute__((hotpatch(0, 0)))
> > #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
> > #define notrace                 __attribute__((patchable_function_entry(0, 0)))
> > #else
> > #define notrace                 __attribute__((__no_instrument_function__))
> > #endif
> > 
> > which I read as three different ways of saying "don't patch here".
> > 
> > Which is confusingly similar or identical to what noinstr does, I do see that
> > noinstr pushes the code to separate section but that in turn might
> > be what __attribute__((__no_instrument_function__)) and
> > friends does?
> > 
> > Are they equivalent?

noinstr is much broader than notrace.  No instrumentation of any kind
(ftrace, kprobes, sanitizers, profilers, etc), in the prologue or body
of the function or its callees, except for regions marked by
instrumentation_{begin,end}().

noinstr is needed in early/late entry code before/after RCU transitions,
as instrumentation code might depend on RCU.  Also, entry code tends to
be sensitive and unable to handle random instrumentation code.

notrace is more narrow, it just means "no function tracing".  It's used
by tracing code and the functions it calls.  It's also used for early
boot code, as ftrace can be enabled on the kernel cmdline.

I believe noinstr is not typically needed for boot code, at least for
the primary CPU.  RCU isn't present yet, and many instrumentations might
not yet be available.  Or their handlers can detect whether they've been
initialized yet.  Or they're disabled in .init sections.

Whether noinstr is needed for *secondary* boot code, I don't know.  It
may be dependent on the instrumentation implementations, e.g., whether
they're enabled globally or per-CPU.  At least on x86, start_secondary()
is notrace.  I don't know enough to say whether that's sufficient.

> > sched_clock_noinstr() is tagged noinstr and sched_clock() is
> > tagged notrace, so there must be a difference here.
> > 
> > secondary_start_kernel() is tagged "notrace" on arm64 but
> > not on arm, should it be tagged "noinstr" or "notrace"?
> > 
> > This kind of stuff makes me uncertain about how this is to be
> > done. A "noinstr vs notrace" section in Documentation/core-api/entry.rst
> > would help a lot I think!

Sounds like a good idea.  We just need a volunteer :-)

-- 
Josh

Re: [GIT PULL] Generic entry for ARM

[Steven Rostedt]

On Mon, 7 Apr 2025 16:47:05 -0700
Josh Poimboeuf <jpoimboe@kernel.org> wrote:

> noinstr is much broader than notrace.  No instrumentation of any kind
> (ftrace, kprobes, sanitizers, profilers, etc), in the prologue or body
> of the function or its callees, except for regions marked by
> instrumentation_{begin,end}().
> 
> noinstr is needed in early/late entry code before/after RCU transitions,
> as instrumentation code might depend on RCU.  Also, entry code tends to
> be sensitive and unable to handle random instrumentation code.

From my understanding, noinstr is usually used in context switching code.
That is, switching from user to kernel and back. It's highly sensitive
and I believe there's CPU helpers that will prevent random code from
happening here (I'm guessing at this, but from the strictness they have
about not doing any instrumentation and how things will break if you do, I
feel that there's going to be a hardware enforcement here, if it's not
already there).

> 
> notrace is more narrow, it just means "no function tracing".  It's used
> by tracing code and the functions it calls.  It's also used for early
> boot code, as ftrace can be enabled on the kernel cmdline.

Right, notrace is basically code that doesn't make sense to trace. Like the
function tracing code itself. Also clocks that the function tracing uses.
You really don't want the clocks being traced as they are used by the
function tracer. Now that we have ftrace_test_recursion_trylock(), it
doesn't crash like it use to. But recursion does cause overhead.

Note, I've been thinking of letting some of the tracing code be traced.
Like the access to files and such. Just because there's been times I want
to know what code is actually being called there (like when I set up a new
histogram!).

> 
> I believe noinstr is not typically needed for boot code, at least for
> the primary CPU.  RCU isn't present yet, and many instrumentations might
> not yet be available.  Or their handlers can detect whether they've been
> initialized yet.  Or they're disabled in .init sections.
> 
> Whether noinstr is needed for *secondary* boot code, I don't know.  It
> may be dependent on the instrumentation implementations, e.g., whether
> they're enabled globally or per-CPU.  At least on x86, start_secondary()
> is notrace.  I don't know enough to say whether that's sufficient.

Not sure, as there's really no context switch at this time. But Peter and
Thomas would know better than I here (continuing to pass the buck!).

> 
> > > sched_clock_noinstr() is tagged noinstr and sched_clock() is
> > > tagged notrace, so there must be a difference here.

sched_clock() is using in tracing. If it wasn't tagged notrace, the
function tracer would be recursing on it.

The noinstr looks like it was created just because some archs call
sched_clock in noinstr code?

> > > 
> > > secondary_start_kernel() is tagged "notrace" on arm64 but
> > > not on arm, should it be tagged "noinstr" or "notrace"?

According to commit b154886f78924:

    arm64: make secondary_start_kernel() notrace
    
    We can't call function trace hook before setup percpu offset.
    When entering secondary_start_kernel(), percpu offset has not
    been initialized.  So this lead hotplug malfunction.
    Here is the flow to reproduce this bug:
    
    echo 0 > /sys/devices/system/cpu/cpu1/online
    echo function > /sys/kernel/debug/tracing/current_tracer
    echo 1 > /sys/kernel/debug/tracing/tracing_on
    echo 1 > /sys/devices/system/cpu/cpu1/online

If other archs crash when doing the above, then sure. Label it notrace ;-)


> > > 
> > > This kind of stuff makes me uncertain about how this is to be
> > > done. A "noinstr vs notrace" section in Documentation/core-api/entry.rst
> > > would help a lot I think!  
> 
> Sounds like a good idea.  We just need a volunteer :-)
> 

Agreed (but not me!)

-- Steve

Re: [GIT PULL] Generic entry for ARM

[Peter Zijlstra]

On Mon, Apr 07, 2025 at 04:47:05PM -0700, Josh Poimboeuf wrote:
> I think Thomas, Peter and Steven are the experts here, but I'll give my
> limited understanding.
> 
> On Mon, Apr 07, 2025 at 10:00:54AM -0700, Paul E. McKenney wrote:
> > > cpu_init() is called from e.g.:
> > > asmlinkage void secondary_start_kernel(struct task_struct *task)
> > > OK should this also be noinstr? Or is that just implied because
> > > of asmlinkage?
> > >
> > > <linux/compiler_types.h> will resolve to:
> > > 
> > > #if defined(CC_USING_HOTPATCH)
> > > #define notrace                 __attribute__((hotpatch(0, 0)))
> > > #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
> > > #define notrace                 __attribute__((patchable_function_entry(0, 0)))
> > > #else
> > > #define notrace                 __attribute__((__no_instrument_function__))
> > > #endif
> > > 
> > > which I read as three different ways of saying "don't patch here".
> > > 
> > > Which is confusingly similar or identical to what noinstr does, I do see that
> > > noinstr pushes the code to separate section but that in turn might
> > > be what __attribute__((__no_instrument_function__)) and
> > > friends does?
> > > 
> > > Are they equivalent?
> 
> noinstr is much broader than notrace.  No instrumentation of any kind
> (ftrace, kprobes, sanitizers, profilers, etc), in the prologue or body
> of the function or its callees, except for regions marked by
> instrumentation_{begin,end}().

Note that noinstr code goes into its own section and various probing
mechanisms (HW breakpoints, kprobes etc..) are explicitly excluded from
touching code in this section.

> noinstr is needed in early/late entry code before/after RCU transitions,
> as instrumentation code might depend on RCU.  Also, entry code tends to
> be sensitive and unable to handle random instrumentation code.

Not strictly RCU, but including RCU. Some of the early entry code might
not have a sane stack or anything much a 'C' environment relies on,
could even not have normal page-tables set up.

noinstr is very much an attempt to stretch the whole 'C-as-assember'
ethos. Only emit the code I write, don't play silly buggers.

Current noinstr usage is indeed interrupt/exception/syscall entry but
also idle loops. Various idle routines disable RCU (which then
prohibits tracing) but some go even further and dismantle much of what a
normal C environment would expect.

> I believe noinstr is not typically needed for boot code, at least for
> the primary CPU.  RCU isn't present yet, and many instrumentations might
> not yet be available.  Or their handlers can detect whether they've been
> initialized yet.  Or they're disabled in .init sections.
> 
> Whether noinstr is needed for *secondary* boot code, I don't know.  It
> may be dependent on the instrumentation implementations, e.g., whether
> they're enabled globally or per-CPU.  At least on x86, start_secondary()
> is notrace.  I don't know enough to say whether that's sufficient.

There might be a case for noinstr there, but as of yet, nobody went and
audited all that. Thus far, I've been the idiot doing all the auditing,
but I think I'll pass on this one for now :-)