[PATCH 5/7] RISC-V: fix auipc-jalr addresses in patched alternatives

[Lad, Prabhakar]

Hi Heiko,

On Tue, Nov 22, 2022 at 11:37 AM Heiko Stübner <heiko at sntech.de> wrote:
>
> Am Dienstag, 22. November 2022, 12:19:40 CET schrieb Heiko Stübner:
> > Am Dienstag, 22. November 2022, 11:59:57 CET schrieb Lad, Prabhakar:
> > > Hi Heiko,
> > >
> > > On Mon, Nov 21, 2022 at 10:17 PM Heiko Stübner <heiko at sntech.de> wrote:
> > > >
> > > > Am Montag, 21. November 2022, 22:31:36 CET schrieb Lad, Prabhakar:
> > > > > Hi Heiko,
> > > > >
> > > <snip>
> > > > As either manually or with a helper like
> > > >
> > > >         https://luplab.gitlab.io/rvcodecjs/#q=0xf4c080e7
> > > >
> > > > you can then decode the actual instruction and compare.
> > > >
> > > > In your log the two jalr instructions decode to different offsets,
> > > >         jalr x1, x1, -180
> > > > vs
> > > >         jalr x1, x1, -834
> > > >
> > > > Can you check what the patch_offset value is in your case?
> > > >
> > > patch_offset for the above case is -654.
> >
> > which is a big indicator that the auipc-jalr-fixup function is not catching
> > the instruction ... i.e. -180 - 654 = -834.
> >
> > I managed to reproduce that issue with your branch now
> > (after hacking up stuff a bit to run in qemu :-) ).
> >
> > I'll try to find out where the fixup fails.
>
> imagine me with a slightly red head now ... as there is a slightly
> embarrassing mistake in the fixup function ;-) .
>
Cheer up now :-)

>
> When going from void* to unsigned int* pointers I have missed
> adjusting the actual patch-location.
>
> The call needs to be
>         patch_text_nosync(alt_ptr + i, call, 8);
>
That did the trick! I have done some limited testing on the board
(even replaced orignal instructions back to nop's even with its
working too).

> instead of the current
>         patch_text_nosync(alt_ptr + i * sizeof(u32), call, 8);
>
> In my str* cases this didn't matter because "i" was 0 there, but in your
> longer assembly it actually patched the wrong location.
>
Ahaa right the alt macro just had calls.

>
> Heiko
>
> ============
> For reference, my debug prints to find where the patching fails was:
>
> diff --git a/arch/riscv/errata/renesas/errata.c b/arch/riscv/errata/renesas/errata.c
> index 986f1c762d72..a5a47c5e9ff8 100644
> --- a/arch/riscv/errata/renesas/errata.c
> +++ b/arch/riscv/errata/renesas/errata.c
> @@ -72,6 +72,7 @@ static void riscv_alternative_fix_auipc_jalr(unsigned int *alt_ptr,
>         u32 rd1;
>
>         for (i = 0; i < num_instr; i++) {
> +printk("%s: looking at inst 0x%x\n", __func__, *(alt_ptr + i));
>                 /* is there a further instruction? */
>                 if (i + 1 >= num_instr)
>                         continue;
> @@ -84,6 +85,7 @@ static void riscv_alternative_fix_auipc_jalr(unsigned int *alt_ptr,
>                 if (rd1 != 1)
>                         continue;
>
> +printk("%s: -> found a auipc + jalr pair\n", __func__);
>                 /* get and adjust new target address */
>                 imm1 = EXTRACT_UTYPE_IMM(*(alt_ptr + i));
>                 imm1 += EXTRACT_ITYPE_IMM(*(alt_ptr + i + 1));
> @@ -101,8 +103,10 @@ static void riscv_alternative_fix_auipc_jalr(unsigned int *alt_ptr,
>                 call[0] |= to_auipc_imm(imm1);
>                 call[1] |= to_jalr_imm(imm1);
>
> +printk("%s: patching to 0x%x and 0x%x\n", __func__, call[0], call[1]);
>                 /* patch the call place again */
> -               patch_text_nosync(alt_ptr + i * sizeof(u32), call, 8);
> +               patch_text_nosync(alt_ptr + i, call, 8);
> +printk("%s: patched to 0x%x and 0x%x\n", __func__, *(alt_ptr + i), *(alt_ptr + i + 1));
>         }
>  }
>
> and then realizing that the "patching to" and "patched to" where different.
>
Thanks for the hunk.
>

Now waiting for your v3. Meanwhile, I'll look into the ALT3() macro.

Cheers,
Prabhakar