[PATCH v3 11/14] RISC-V: fix auipc-jalr addresses in patched alternatives

[Andrew Jones]

On Wed, Dec 07, 2022 at 10:35:50AM +0100, Heiko Stübner wrote:
> Am Dienstag, 6. Dezember 2022, 23:10:01 CET schrieb Heiko Stübner:
> > Am Donnerstag, 1. Dezember 2022, 20:33:53 CET schrieb Andrew Jones:
> > > On Wed, Nov 30, 2022 at 11:56:11PM +0100, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <heiko.stuebner at vrull.eu>
...
> > > > +#define to_auipc_imm(value)						\
> > > > +	((value & JALR_SIGN_MASK) ?					\
> > > > +	((value & RV_U_IMM_31_12_MASK) + AUIPC_PAD) :	\
> > > > +	(value & RV_U_IMM_31_12_MASK))
> > > 
> > > I know RV_U_IMM_31_12_OPOFF is 0, but it looks odd not shifting
> > > RV_U_IMM_31_12_MASK when we do shift RV_I_IMM_11_0_MASK.
> > > 
> > > So, it looks like to_auipc_imm() is doing
> > > 
> > >    offset[31:12] + ((value & BIT(11)) ? (1 << 12) : 0)
> > > 
> > > but the spec says the auipc part of the 'call' pseudoinstruction should be
> > 
> > can you point me to that part of the spec?
> > 
> > For educational purposes, because in the main riscv-spec I only found
> > the main auipc + jalr descriptions, but nothing about the actual
> > "call" pseudoinstruction.
> > 
> > [and I'm probably just looking at the wrong document]
> > 
> > 
> > >    offset[31:12] + offset[11]
> > >  
> > > which I think would be written as
> > > 
> > >  ((((value) & RV_U_IMM_31_12_MASK) << RV_U_IMM_31_12_OPOFF) + ((value) & BIT(11)))
> > > 
> > > or what am I missing?
> 
> So far I've found the riscv-asm-manual [0], which only states for call
> 	auipc x1, offset[31:12]
> 	jalr x1, x1, offset[11:0]
> 
> and the psABI spec [1], neither mention your "offset[31:12] + offset[11]" ?
> 
> But [1] does contain that tiny sentence
> "The successive instruction has a signed 12-bit immediate so the value
>  of the preceding high 20-bit relocation may have 1 added to it."
> 
> 
> I.e. the lower 12 bits become themself a signed number [-2048:2047]
> 
> Take an example:
> - address is 1862116 ( 0b 111000110 100111100100 )
> - address[31:12] becomes 1859584 (as 0b 111000110 000000000000)
> - while address[11:0] is 2532 as part of the bigger number
> - as lone 12bit signed IMM it becomes -1564
> - so you need to add this 4096 to the auipc IMM to compensate
> 
> 
> Heiko
> 
> 
> [0] https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
> [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0

Yeah, I got this figured out yesterday and sent another mail confirming
the way you had it was right. Did you receive that mail?

Where I see 

 call offset     auipc x1, offset[31 : 12] + offset[11]
                 jalr x1, offset[11:0](x1)

is in chapter 25 of the unprivileged ISA.

> 
> > 
> > that whole thing came from the ftrace parts, also doing call fixup voodoo
> > And I can't really say that I understand every nook and cranny of it.
> > 
> > But for practical purposes, the addresses generated now work,
> > and also seem to work for the ftrace counterpart (see include/asm/ftrace.h)
> > 
> > [another place that will profit from more generalization :-) ]
> > 
> > 
> > > > +
> > > > +void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len,
> > > > +				      int patch_offset)
> > > > +{
> > > > +	int num_instr = len / sizeof(u32);
> > > > +	unsigned int call[2];
> > > > +	int i;
> > > > +	int imm;
> > > > +	u32 rd1;
> > > > +
> > > > +	/*
> > > > +	 * stop one instruction before the end, as we're checking
> > > > +	 * for auipc + jalr
> > > > +	 */
> > > > +	for (i = 0; i < num_instr - 1; i++) {
> > > 
> > > If we change riscv_instruction_at() to just take a pointer then we can do
> > > the math in the for() and actually just use pointer arithmetic.
> > > 
> > >         uint32_t *p = alt_ptr;
> > >         for (i = 0; i < num_instr - 1; i++, p++) {
> > 
> > Wasn't not using uint32 pointers the whole point of going with the accessor?

Oh, right. So maybe stick to the offsetting, but still do the math in the
caller.

Thanks,
drew