Re: [PATCH v2 02/16] riscv: alternatives: Use generated instruction headers for patching code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect immediate calculation for auipc unconditionally adds 4096, breaking alternative patching for jumps where bit 11 is 0.
- [Low] Using a jalr-specific extraction function on an auipc instruction.
--

commit 67f209569c2cc2afe918708c4cf86d391c4abcd2
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>
Subject: riscv: alternatives: Use generated instruction headers for patching code

This commit migrates the alternative patching code to use generated
instruction headers instead of hand-written composition functions.

> diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
> --- a/arch/riscv/kernel/alternative.c
> +++ b/arch/riscv/kernel/alternative.c
[ ... ]
> @@ -78,14 +79,24 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn,
>  					     u32 jalr_insn, int patch_offset)
>  {
>  	u32 call[2] = { auipc_insn, jalr_insn };
> +	u32 auipc_imm;
>  	s32 imm;
>  
>  	/* get and adjust new target address */
> -	imm = riscv_insn_extract_utype_itype_imm(auipc_insn, jalr_insn);
> +	imm = riscv_insn_auipc_extract_imm(auipc_insn) + riscv_insn_jalr_extract_imm(jalr_insn);
>  	imm -= patch_offset;
>  
> +	/*
> +	 * When the 32-bit immediate is split across auipc and jalr, the
> +	 * constructed immediates need to be treated as individually sign
> +	 * extended numbers. Add the sign bit of the lower 12 bits to the upper
> +	 * 20 bits to undo the bleeding of the sign.
> +	 */
> +	auipc_imm = imm + (BIT(11) << 1);

[Severity: High]
Does this unconditionally add 4096 to the immediate?

The original code conditionally added the offset using (imm & BIT(11)) << 1,
but the new code appears to evaluate to a constant BIT(11) << 1. If imm
has bit 11 set to 0, won't this corrupt the target jump address by 4096 bytes?

> +
>  	/* update instructions */
> -	riscv_insn_insert_utype_itype_imm(&call[0], &call[1], imm);
> +	riscv_insn_auipc_insert_imm(&call[0], auipc_imm);
> +	riscv_insn_jalr_insert_imm(&call[1], imm);
[ ... ]
> @@ -127,7 +138,7 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len,
>  				continue;
>  
>  			/* if instruction pair is a call, it will use the ra register */
> -			if (RV_EXTRACT_RD_REG(insn) != 1)
> +			if (riscv_insn_jalr_extract_xd(insn) != 1)

[Severity: Low]
Since insn is verified to be an auipc instruction just above this block,
should this be using riscv_insn_auipc_extract_xd(insn) instead?

While they both extract the destination register at bits 11:7 and work
mathematically, using the jalr helper on an auipc instruction seems
to break the abstraction provided by the new headers.

>  				continue;
>  
>  			riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=2