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

[Emil Renner Berthing]

On Wed, 7 Dec 2022 at 11:44, Andrew Jones <ajones at ventanamicro.com> wrote:
>
> 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.

Sorry, for being late to the conversation. We've fixed confusion about
this auipc/jalr offset at least once before. Maybe that explanation
there is helpful to someone:
https://git.kernel.org/torvalds/c/0966d385830de3470b7131db8e86c0c5bc9c52dc

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