[PATCH v1 1/1] riscv: fix runtime constant support for nommu kernels

[Charles Mirabile]

On Fri, May 30, 2025 at 10:35 PM Charlie Jenkins <charlie at rivosinc.com> wrote:
>
> On Fri, May 30, 2025 at 05:14:22PM -0400, Charles Mirabile wrote:
> > the `__runtime_fixup_32` function does not handle the case where `val` is
> > zero correctly (as might occur when patching a nommu kernel and referring
> > to a physical address below the 4GiB boundary whose upper 32 bits are all
> > zero) because nothing in the existing logic prevents the code from taking
> > the `else` branch of both nop-checks and emitting two `nop` instructions.
> >
> > This leaves random garbage in the register that is supposed to receive the
> > upper 32 bits of the pointer instead of zero that when combined with the
> > value for the lower 32 bits yields an invalid pointer and causes a kernel
> > panic when that pointer is eventually accessed.
> >
> > The author clearly considered the fact that if the `lui` is converted into
> > a `nop` that the second instruction needs to be adjusted to become an `li`
> > instead of an `addi`, hence introducing the `addi_insn_mask` variable, but
> > didn't follow that logic through fully to the case where the `else` branch
> > executes. To fix it just adjust the logic to ensure that the second `else`
> > branch is not taken if the first instruction will be patched to a `nop`.
>
> You have an accurate assesment here, I missed the zero case :/.
> Thank you for fixing the issue!
>
> >
> > Fixes: a44fb5722199 ("riscv: Add runtime constant support")
> >
> > Signed-off-by: Charles Mirabile <cmirabil at redhat.com>
> > ---
> >  arch/riscv/include/asm/runtime-const.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/runtime-const.h b/arch/riscv/include/asm/runtime-const.h
> > index 451fd76b8811..d766e2b9e6df 100644
> > --- a/arch/riscv/include/asm/runtime-const.h
> > +++ b/arch/riscv/include/asm/runtime-const.h
> > @@ -206,7 +206,7 @@ static inline void __runtime_fixup_32(__le16 *lui_parcel, __le16 *addi_parcel, u
> >               addi_insn_mask &= 0x07fff;
> >       }
> >
> > -     if (lower_immediate & 0x00000fff) {
> > +     if (lower_immediate & 0x00000fff || lui_insn == RISCV_INSN_NOP4) {
>
> This comment is borderline too nitpicky so feel free to dismiss it :).
> It's slightly wasteful to have this check right after the if-statement
I agree. Your patch definitely works, but the complexity starts to get
kind of hairy though to handle it correctly. Especially given this is
the patching code that only runs once and is not in the hot path.
> that sets it. I am not sure what the most readable way of doing this is
> though. What would you think about a patch like the following instead?
>
> From 1c56536c1e338735140c9090f06da49a3d245a61 Mon Sep 17 00:00:00 2001
> From: Charlie Jenkins <charlie at rivosinc.com>
> Date: Fri, 30 May 2025 19:25:13 -0700
> Subject: [PATCH] alternate fix
>
> ---
>  arch/riscv/include/asm/runtime-const.h | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/riscv/include/asm/runtime-const.h b/arch/riscv/include/asm/runtime-const.h
> index 451fd76b8811..085a0bb26fbb 100644
> --- a/arch/riscv/include/asm/runtime-const.h
> +++ b/arch/riscv/include/asm/runtime-const.h
> @@ -179,12 +179,9 @@ static inline void __runtime_fixup_caches(void *where, unsigned int insns)
>  static inline void __runtime_fixup_32(__le16 *lui_parcel, __le16 *addi_parcel, unsigned int val)
>  {
>         unsigned int lower_immediate, upper_immediate;
> -       u32 lui_insn, addi_insn, addi_insn_mask;
> +       u32 lui_insn, addi_insn;
>         __le32 lui_res, addi_res;
>
> -       /* Mask out upper 12 bit of addi */
> -       addi_insn_mask = 0x000fffff;
> -
>         lui_insn = (u32)le16_to_cpu(lui_parcel[0]) | (u32)le16_to_cpu(lui_parcel[1]) << 16;
>         addi_insn = (u32)le16_to_cpu(addi_parcel[0]) | (u32)le16_to_cpu(addi_parcel[1]) << 16;
>
> @@ -195,6 +192,15 @@ static inline void __runtime_fixup_32(__le16 *lui_parcel, __le16 *addi_parcel, u
>                 /* replace upper 20 bits of lui with upper immediate */
>                 lui_insn &= 0x00000fff;
>                 lui_insn |= upper_immediate & 0xfffff000;
> +
> +               if (lower_immediate & 0x00000fff) {
> +                       /* replace upper 12 bits of addi with lower 12 bits of val */
> +                       addi_insn &= 0x000fffff;
> +                       addi_insn |= (lower_immediate & 0x00000fff) << 20;
> +               } else {
> +                       /* replace addi with nop if lower_immediate is empty */
> +                       addi_insn = RISCV_INSN_NOP4;
> +               }
>         } else {
>                 /* replace lui with nop if immediate is small enough to fit in addi */
>                 lui_insn = RISCV_INSN_NOP4;
> @@ -203,16 +209,9 @@ static inline void __runtime_fixup_32(__le16 *lui_parcel, __le16 *addi_parcel, u
>                  * is performed by adding with the x0 register. Setting rs to
>                  * zero with the following mask will accomplish this goal.
>                  */
> -               addi_insn_mask &= 0x07fff;
> -       }
> -
> -       if (lower_immediate & 0x00000fff) {
> +               addi_insn &= 0x07fff;
>                 /* replace upper 12 bits of addi with lower 12 bits of val */
> -               addi_insn &= addi_insn_mask;
>                 addi_insn |= (lower_immediate & 0x00000fff) << 20;
> -       } else {
> -               /* replace addi with nop if lower_immediate is empty */
> -               addi_insn = RISCV_INSN_NOP4;
>         }
>
>         addi_res = cpu_to_le32(addi_insn);
> --
> 2.43.0
>
> Let me know what you think!
Frankly, I wonder whether this whole optimization of replacing `lui` or
`addiw` with `nop` is even worth it. This isn't like linker relaxation
where we can actually change the amount of code by eliding an instruction.
Is `nop` actually that much faster to execute than `lui` or `addiw` to
justify the complexity?
>
> - Charlie
>
Best - Charlie