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

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

Hi,

Am Montag, 21. November 2022, 10:50:09 CET schrieb Lad, Prabhakar:
> On Thu, Nov 10, 2022 at 4:50 PM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> >
> > Alternatives live in a different section, so addresses used by call
> > functions will point to wrong locations after the patch got applied.
> >
> > Similar to arm64, adjust the location to consider that offset.
> >
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > ---

[...]

> I have the below assembly code which I have tested without the
> alternatives for the RZ/Five CMO,
> 
> #define ALT_CMO_OP(_op, _start, _size, _cachesize, _dir, _ops)        \
> asm volatile(".option push\n\t\n\t"                    \
>          ".option norvc\n\t"                    \
>          ".option norelax\n\t"                    \
>          "addi sp,sp,-16\n\t"                    \
>          "sd    s0,0(sp)\n\t"                    \
>          "sd    ra,8(sp)\n\t"                    \
>          "addi    s0,sp,16\n\t"                    \
>          "mv a4,%6\n\t"                        \
>          "mv a3,%5\n\t"                        \
>          "mv a2,%4\n\t"                        \
>          "mv a1,%3\n\t"                        \
>          "mv a0,%0\n\t"                        \
>          "call rzfive_cmo\n\t"                    \
>          "ld    ra,8(sp)\n\t"                    \
>          "ld    s0,0(sp)\n\t"                    \
>          "addi    sp,sp,16\n\t"                    \
>          ".option pop\n\t"                        \
>          : : "r"(_cachesize),                    \
>          "r"((unsigned long)(_start) & ~((_cachesize) - 1UL)),    \
>          "r"((unsigned long)(_start) + (_size)),            \
>          "r"((unsigned long) (_start)),                \
>          "r"((unsigned long) (_size)),                \
>          "r"((unsigned long) (_dir)),                \
>          "r"((unsigned long) (_ops))                \
>          : "a0", "a1", "a2", "a3", "a4", "memory")
>
> Now when integrate this with ALTERNATIVE_2() as below,
> 
> #define ALT_CMO_OP(_op, _start, _size, _cachesize, _dir, _ops)        \
> asm volatile(ALTERNATIVE_2(                        \
>     __nops(14),                            \
>     "mv a0, %1\n\t"                            \
>     "j 2f\n\t"                            \
>     "3:\n\t"                            \
>     "cbo." __stringify(_op) " (a0)\n\t"                \
>     "add a0, a0, %0\n\t"                        \
>     "2:\n\t"                            \
>     "bltu a0, %2, 3b\n\t"                        \
>     __nops(8), 0, CPUFEATURE_ZICBOM, CONFIG_RISCV_ISA_ZICBOM,    \
>     ".option push\n\t\n\t"                        \
>     ".option norvc\n\t"                        \
>     ".option norelax\n\t"                        \
>     "addi sp,sp,-16\n\t"                        \
>     "sd    s0,0(sp)\n\t"                        \
>     "sd    ra,8(sp)\n\t"                        \
>     "addi    s0,sp,16\n\t"                        \
>     "mv a4,%6\n\t"                            \
>     "mv a3,%5\n\t"                            \
>     "mv a2,%4\n\t"                            \
>     "mv a1,%3\n\t"                            \
>     "mv a0,%0\n\t"                            \
>     "call rzfive_cmo\n\t"                \
>     "ld    ra,8(sp)\n\t"                        \
>     "ld    s0,0(sp)\n\t"                        \
>     "addi    sp,sp,16\n\t"                        \
>     ".option pop\n\t"                        \
>     , ANDESTECH_VENDOR_ID,                        \
>             ERRATA_ANDESTECH_NO_IOCP, CONFIG_ERRATA_RZFIVE_CMO)    \
>     : : "r"(_cachesize),                        \
>     "r"((unsigned long)(_start) & ~((_cachesize) - 1UL)),    \
>     "r"((unsigned long)(_start) + (_size)),            \
>     "r"((unsigned long) (_start)),                \
>     "r"((unsigned long) (_size)),                \
>     "r"((unsigned long) (_dir)),                \
>     "r"((unsigned long) (_ops))                \
>     : "a0", "a1", "a2", "a3", "a4", "memory")
> 
> I am seeing kernel panic with this change. Looking at the
> riscv_alternative_fix_auipc_jalr() implementation it assumes the rest
> of the alternative options are calls too. Is my understanding correct
> here?

The loop walks through the instructions after the location got patched and
checks if an instruction is an auipc and the next one is a jalr and only then
adjusts the address accordingly.

So it _should_ leave all other (non auipc+jalr) instructions alone.
(hopefully)


> Do you think this is the correct approach in my case?

It does look correct on first glance.

As I had that passing thought, are you actually calling
	riscv_alternative_fix_auipc_jalr()
from your errata/.../foo.c after doing the patching?

I.e. with the current patchset, that function is only called from the
cpufeature part, but for example not from the other patching locations.
[and a future revision should probably change that :-) ]


After making sure that function actually runs, the next thing you could try
is to have both the "original" code and the patch be identical, i.e.
replace the cbo* part with your code as well and then just output the
instructions via printk to check what the addresses do in both.

After riscv_alternative_fix_auipc_jalr() ran then both code variants
should be identical when using the same code in both areas.


> Note, I wanted to test with ALTERNATIVE_2() first to make sure
> everything is okay and then later test my ALTERNATIVE_3()
> implementation.

sounds like a very sensible idea to use the existing macros
first for verification :-)


Heiko



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