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

["Heiko Stübner" <heiko@sntech.de>]

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@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 ;-) .


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

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.


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.




_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv