Re: [PATCH 4/4] arm64: alternatives: Make branch and ADRP offset helpers noinst

[Jisheng Zhang <jszhang@kernel.org>]

On Sun, Nov 08, 2026 at 07:43:58AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] arm64: insn: branch profiling instrumentation in noinstr
>   offset helpers
> --
> 
> commit b08429609214cdffaa257c9ef953b28e179973db
> Author: Jisheng Zhang <jszhang@kernel.org>
> 
> arm64: alternatives: Make branch and ADRP offset helpers noinst
> 
> This commit marks the branch and ADRP offset helpers noinstr and introduces
> __always_inline immediate encoding and decoding helpers so that
> patch_alternative() does not call instrumentable code while modifying
> instructions.
> 
> > diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> > index e70ac02385153..b98610fcab32e 100644
> > --- a/arch/arm64/lib/insn.c
> > +++ b/arch/arm64/lib/insn.c
> 
> [ ... ]
> 
> > @@ -1299,23 +1316,23 @@ u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
> >   * signed value (so it can be used when computing a new branch
> >   * target).
> >   */
> > -s32 aarch64_get_branch_offset(u32 insn)
> > +s32 noinstr aarch64_get_branch_offset(u32 insn)
> >  {
> >  	s32 imm;
> >  
> >  	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn)) {
> > -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
> > +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
> >  		return (imm << 6) >> 4;
> >  	}
> >  
> >  	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
> >  	    aarch64_insn_is_bcond(insn)) {
> > -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_19, insn);
> > +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_19, insn);
> >  		return (imm << 13) >> 11;
> >  	}
> >  
> >  	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn)) {
> > -		imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_14, insn);
> > +		imm = __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_14, insn);
> >  		return (imm << 18) >> 16;
> >  	}
> >  
> > @@ -1327,35 +1344,35 @@ s32 aarch64_get_branch_offset(u32 insn)
> >   * Encode the displacement of a branch in the imm field and return the
> >   * updated instruction.
> >   */
> > -u32 aarch64_set_branch_offset(u32 insn, s32 offset)
> > +u32 noinstr aarch64_set_branch_offset(u32 insn, s32 offset)
> >  {
> >  	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn))
> > -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
> > +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_26, insn,
> >  						     offset >> 2);
> >  
> >  	if (aarch64_insn_is_cbz(insn) || aarch64_insn_is_cbnz(insn) ||
> >  	    aarch64_insn_is_bcond(insn))
> > -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_19, insn,
> > +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_19, insn,
> >  						     offset >> 2);
> >  
> >  	if (aarch64_insn_is_tbz(insn) || aarch64_insn_is_tbnz(insn))
> > -		return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_14, insn,
> > +		return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_14, insn,
> >  						     offset >> 2);
> >  
> >  	/* Unhandled instruction */
> >  	BUG();
> >  }
> >  
> > -s32 aarch64_insn_adrp_get_offset(u32 insn)
> > +s32 noinstr aarch64_insn_adrp_get_offset(u32 insn)
> >  {
> >  	BUG_ON(!aarch64_insn_is_adrp(insn));
> > -	return aarch64_insn_decode_immediate(AARCH64_INSN_IMM_ADR, insn) << 12;
> > +	return __aarch64_insn_decode_immediate(AARCH64_INSN_IMM_ADR, insn) << 12;
> >  }
> >  
> > -u32 aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
> > +u32 noinstr aarch64_insn_adrp_set_offset(u32 insn, s32 offset)
> >  {
> >  	BUG_ON(!aarch64_insn_is_adrp(insn));
> > -	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_ADR, insn,
> > +	return __aarch64_insn_encode_immediate(AARCH64_INSN_IMM_ADR, insn,
> >  						offset >> 12);
> >  }
> 
> [Severity: Medium]
> Can these four noinstr helpers still call into ftrace_likely_update() when
> CONFIG_TRACE_BRANCH_PROFILING is enabled?
> 
> Although f22c81bebf8b ("arm64: Disable branch profiling for all arm64 code")
> added -DDISABLE_BRANCH_PROFILING to arch/arm64/Kbuild, arch/arm64/lib/ is
> built via libs-y in arch/arm64/Makefile rather than arch/arm64/Kbuild, so
> arch/arm64/lib/insn.o is compiled without DISABLE_BRANCH_PROFILING.
> 
> As a result, the generic BUG_ON() in aarch64_insn_adrp_get_offset() and
> aarch64_insn_adrp_set_offset() expands to unlikely(), and the if statements
> in aarch64_get_branch_offset() and aarch64_set_branch_offset() are
> instrumented under CONFIG_PROFILE_ALL_BRANCHES.  When patch_alternative()
> calls these helpers while patching live instructions before cache
> maintenance completes, they call the instrumentable ftrace_likely_update().
> 
> Passing -DDISABLE_BRANCH_PROFILING in arch/arm64/lib/Makefile (or defining
> DISABLE_BRANCH_PROFILING in arch/arm64/lib/insn.c) would prevent branch
> profiling instrumentation here.

All comments should have been addressed in v2. Plz review

Thanks very much for the comments and hints!