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

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Heiko,

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>
> ---
>  arch/riscv/kernel/cpufeature.c | 79 +++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 694267d1fe81..026512ca9c4c 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -298,6 +298,74 @@ static u32 __init_or_module cpufeature_probe(unsigned int stage)
>         return cpu_req_feature;
>  }
>
> +#include <asm/parse_asm.h>
> +
> +DECLARE_INSN(jalr, MATCH_JALR, MASK_JALR)
> +DECLARE_INSN(auipc, MATCH_AUIPC, MASK_AUIPC)
> +
> +static inline bool is_auipc_jalr_pair(long insn1, long insn2)
> +{
> +       return is_auipc_insn(insn1) && is_jalr_insn(insn2);
> +}
> +
> +#define JALR_SIGN_MASK         BIT(I_IMM_SIGN_OPOFF - I_IMM_11_0_OPOFF)
> +#define JALR_OFFSET_MASK       I_IMM_11_0_MASK
> +#define AUIPC_OFFSET_MASK      U_IMM_31_12_MASK
> +#define AUIPC_PAD              (0x00001000)
> +#define JALR_SHIFT             I_IMM_11_0_OPOFF
> +
> +#define to_jalr_imm(offset)                                            \
> +       ((offset & I_IMM_11_0_MASK) << I_IMM_11_0_OPOFF)
> +
> +#define to_auipc_imm(offset)                                           \
> +       ((offset & JALR_SIGN_MASK) ?                                    \
> +       ((offset & AUIPC_OFFSET_MASK) + AUIPC_PAD) :    \
> +       (offset & AUIPC_OFFSET_MASK))
> +
> +static void riscv_alternative_fix_auipc_jalr(unsigned int *alt_ptr,
> +                                            unsigned int len, int patch_offset)
> +{
> +       int num_instr = len / sizeof(u32);
> +       unsigned int call[2];
> +       int i;
> +       int imm1;
> +       u32 rd1;
> +
> +       for (i = 0; i < num_instr; i++) {
> +               /* is there a further instruction? */
> +               if (i + 1 >= num_instr)
> +                       continue;
> +
> +               if (!is_auipc_jalr_pair(*(alt_ptr + i), *(alt_ptr + i + 1)))
> +                       continue;
> +
> +               /* call will use ra register */
> +               rd1 = EXTRACT_RD_REG(*(alt_ptr + i));
> +               if (rd1 != 1)
> +                       continue;
> +
> +               /* get and adjust new target address */
> +               imm1 = EXTRACT_UTYPE_IMM(*(alt_ptr + i));
> +               imm1 += EXTRACT_ITYPE_IMM(*(alt_ptr + i + 1));
> +               imm1 -= patch_offset;
> +
> +               /* pick the original auipc + jalr */
> +               call[0] = *(alt_ptr + i);
> +               call[1] = *(alt_ptr + i + 1);
> +
> +               /* drop the old IMMs */
> +               call[0] &= ~(U_IMM_31_12_MASK);
> +               call[1] &= ~(I_IMM_11_0_MASK << I_IMM_11_0_OPOFF);
> +
> +               /* add the adapted IMMs */
> +               call[0] |= to_auipc_imm(imm1);
> +               call[1] |= to_jalr_imm(imm1);
> +
> +               /* patch the call place again */
> +               patch_text_nosync(alt_ptr + i * sizeof(u32), call, 8);
> +       }
> +}
> +

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?

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

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

Cheers,
Prabhakar

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