[PATCH v1 0/1] fix riscv runtime constant support

[PATCH v1 0/1] fix riscv runtime constant support

[Charles Mirabile]

I discovered that something broke basic booting on riscv64 for a nommu
kernel with a minimal configuration running on qemu between 6.13 and
current master. The symptom was that the kernel would hang and print
nothing instead of booting normally. I bisected my way to:

commit a44fb5722199 ("riscv: Add runtime constant support")

Analyzing in a debugger, I was able to conclude that the bug was occurring
due to an invalid pointer dereference in `__d_lookup_rcu` trying to access
`dentry_cache`. That variable was at 0x8040f480 but the upper half of the
actual pointer value it was trying to access was filled with garbage.

Looking at the disassembly I saw that in the patched instructions that a
`nop` instruction had replaced both the `lui` and the `addiw` that were
supposed to create the upper half of the pointer so the register was not
initialized. The code responsible for patching does not ensure that at
least one instruction is not replaced with a `nop` if `val` is zero.

To reproduce the bug the following minimal config and initrd can be used:

$ cat ../minimal.config
CONFIG_EXPERT=y
CONFIG_NONPORTABLE=y
CONFIG_KERNEL_UNCOMPRESSED=y
CONFIG_RISCV_M_MODE=y
CONFIG_PRINTK=y
CONFIG_TTY=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_BINFMT_ELF_FDPIC=y
CONFIG_POWER_RESET=y
CONFIG_POWER_RESET_SYSCON=y
CONFIG_POWER_RESET_SYSCON_POWEROFF=y
CONFIG_DEBUG_INFO_DWARF5=y
$ cat ../init.s
.text
.global _start
_start:
	li a0, 1
	la a1, .Lmsg
	lui a2, %hi(.Lmsglen)
	addi a2, a2, %lo(.Lmsglen)
	li a7, 64	  # __NR_write
	ecall
	li a0, 0xfee1dead
	li a1, 0x28121969
	li a2, 0x4321fedc # CMD_HALT
	li a7, 142	  # __NR_reboot
	ecall
	unimp
.data
.Lmsg:
.ascii "Hello!\n"
.Lmsglen = . - .Lmsg
$ mkdir ../rootfs
$ riscv64-linux-gnu-gcc -static -shared \
 -ffreestanding -nostdlib -march=rv64i -mabi=lp64 \
 ../init.s -o ../rootfs/init
$ cd ../rootfs && find . | cpio -co > ../rootfs.cpio && cd - >/dev/null
13 blocks
$ export CROSS_COMPILE=riscv64-linux-gnu- ARCH=riscv
$ make KCONFIG_ALLCONFIG=../minimal.config allnoconfig
$ make -j $(nproc)
...
  Kernel: arch/riscv/boot/Image is ready
$ qemu-system-riscv64 -cpu rv64,mmu=off -machine virt -bios none \
 -nographic -no-reboot -net none \
 -kernel arch/riscv/boot/Image -initrd ../rootfs.cpio
...
Run /init as init process
Hello!
reboot: Power down

On current master, nothing will be printed and the qemu command will just
hang (kill with control+a x), but with this patch it will boot normally.

Signed-off-by: Charles Mirabile <cmirabil@redhat.com>

Charles Mirabile (1):
  riscv: fix runtime constant support for nommu kernels

 arch/riscv/include/asm/runtime-const.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.49.0


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

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

[Charles Mirabile]

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`.

Fixes: a44fb5722199 ("riscv: Add runtime constant support")

Signed-off-by: Charles Mirabile <cmirabil@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) {
 		/* replace upper 12 bits of addi with lower 12 bits of val */
 		addi_insn &= addi_insn_mask;
 		addi_insn |= (lower_immediate & 0x00000fff) << 20;
-- 
2.49.0


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

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

[Charlie Jenkins]

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@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
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@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!

- Charlie


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

Re: [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@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@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@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


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

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

[Charles Mirabile]

From: Charlie Jenkins <charlie@rivosinc.com>

To be clear, I am suggesting that the following patch to just rip out all
of the if else stuff would also fix this bug, but maybe the perf gains of
potentially inserting nops is worth it.

---
 arch/riscv/include/asm/runtime-const.h | 33 ++++++--------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/arch/riscv/include/asm/runtime-const.h b/arch/riscv/include/asm/runtime-const.h
index 451fd76b8811..da47253a89a9 100644
--- a/arch/riscv/include/asm/runtime-const.h
+++ b/arch/riscv/include/asm/runtime-const.h
@@ -179,41 +179,22 @@ 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;
 
 	lower_immediate = sign_extend32(val, 11);
 	upper_immediate = (val - lower_immediate);
 
-	if (upper_immediate & 0xfffff000) {
-		/* replace upper 20 bits of lui with upper immediate */
-		lui_insn &= 0x00000fff;
-		lui_insn |= upper_immediate & 0xfffff000;
-	} else {
-		/* replace lui with nop if immediate is small enough to fit in addi */
-		lui_insn = RISCV_INSN_NOP4;
-		/*
-		 * lui is being skipped, so do a load instead of an add. A load
-		 * is performed by adding with the x0 register. Setting rs to
-		 * zero with the following mask will accomplish this goal.
-		 */
-		addi_insn_mask &= 0x07fff;
-	}
+	/* 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 &= addi_insn_mask;
-		addi_insn |= (lower_immediate & 0x00000fff) << 20;
-	} else {
-		/* replace addi with nop if lower_immediate is empty */
-		addi_insn = RISCV_INSN_NOP4;
-	}
+	/* replace upper 12 bits of addi with lower 12 bits of val */
+	addi_insn &= 0x000fffff;
+	addi_insn |= (lower_immediate & 0x00000fff) << 20;
 
 	addi_res = cpu_to_le32(addi_insn);
 	lui_res = cpu_to_le32(lui_insn);
-- 
2.49.0


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

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

[Charlie Jenkins]

On Fri, May 30, 2025 at 10:54:23PM -0400, Charles Mirabile wrote:
> On Fri, May 30, 2025 at 10:35 PM Charlie Jenkins <charlie@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@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.

Yeah you are right it starts to become overkill...

> > 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@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?

The complexity is pretty minimal (and already implemented) so there
doesn't seem to be enough justification to remove the optimization. Yes,
it will have minimal performance impact, but I don't have the numbers to
be able to confidently remove the nops. This code patching is happening
for code paths that get executed _very_ frequently so any optimization
can potentially have a large impact over the course of a long running
program.

Let's go forward with your original proposal for now, that will keep the
code the simplest but still have the optimization of using the nop.

- Charlie

> >
> > - Charlie
> >
> Best - Charlie
> 

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

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

[Charlie Jenkins]

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`.
> 
> Fixes: a44fb5722199 ("riscv: Add runtime constant support")
> 
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.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) {
>  		/* replace upper 12 bits of addi with lower 12 bits of val */
>  		addi_insn &= addi_insn_mask;
>  		addi_insn |= (lower_immediate & 0x00000fff) << 20;
> -- 
> 2.49.0
> 

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

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

[Palmer Dabbelt]

On Fri, 30 May 2025 14:14:22 PDT (-0700), cmirabil@redhat.com 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`.
>
> Fixes: a44fb5722199 ("riscv: Add runtime constant support")
>
> Signed-off-by: Charles Mirabile <cmirabil@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) {
>  		/* replace upper 12 bits of addi with lower 12 bits of val */
>  		addi_insn &= addi_insn_mask;
>  		addi_insn |= (lower_immediate & 0x00000fff) << 20;

Thanks.  This looks reasonable to me, so I've stuck it on fixes -- I'm 
still sorting out some post-merge-window cruft, so it might take a bit 
to show up for real.

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

Re: [PATCH v1 0/1] fix riscv runtime constant support

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@dabbelt.com>:

On Fri, 30 May 2025 17:14:21 -0400 you wrote:
> I discovered that something broke basic booting on riscv64 for a nommu
> kernel with a minimal configuration running on qemu between 6.13 and
> current master. The symptom was that the kernel would hang and print
> nothing instead of booting normally. I bisected my way to:
> 
> commit a44fb5722199 ("riscv: Add runtime constant support")
> 
> [...]

Here is the summary with links:
  - [v1,1/1] riscv: fix runtime constant support for nommu kernels
    https://git.kernel.org/riscv/c/8d90d9872eda

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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