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

Andrew Jones
2022-12-07T10:45:11Z

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@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