1. Patchew
  2. linux
  3. [PATCH v9 0/2] riscv: Add runtime constant support
  4. View patch

[PATCH v9 1/2] riscv: Move nop definition to insn-def.h

Charlie Jenkins posted 2 patches 9 months ago
There is a newer version of this series
[PATCH v9 1/2] riscv: Move nop definition to insn-def.h
Posted by Charlie Jenkins 9 months ago 
We have duplicated the definition of the nop instruction in ftrace.h and
in jump_label.c. Move this definition into the generic file insn-def.h
so that they can share the definition with each other and with future
files.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/include/asm/ftrace.h   | 1 -
 arch/riscv/include/asm/insn-def.h | 2 ++
 arch/riscv/kernel/ftrace.c        | 6 +++---
 arch/riscv/kernel/jump_label.c    | 4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index c4721ce44ca474654b37b3d51bc0a63d46bc1eff..b7f361a50f6445d02a0d88eef5547ee27c1fb52e 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -79,7 +79,6 @@ struct dyn_arch_ftrace {
 #define AUIPC_RA		(0x00000097)
 #define JALR_T0			(0x000282e7)
 #define AUIPC_T0		(0x00000297)
-#define NOP4			(0x00000013)
 
 #define to_jalr_t0(offset)						\
 	(((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index 9a913010cdd93cdfdd93f467e7880e20cce0dd2b..0a1fc5134f29da190554c59f8cee3b5374c9aa2d 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -200,4 +200,6 @@
 #define ZAWRS_WRS_NTO	".4byte 0x00d00073"
 #define ZAWRS_WRS_STO	".4byte 0x01d00073"
 
+#define RISCV_INSN_NOP4	0x00000013U
+
 #endif /* __ASM_INSN_DEF_H */
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 3524db5e4fa014a4594465f849d898a030bfb7b8..674dcdfae7a149c339f1e791adb450535f22991b 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -36,7 +36,7 @@ static int ftrace_check_current_call(unsigned long hook_pos,
 				     unsigned int *expected)
 {
 	unsigned int replaced[2];
-	unsigned int nops[2] = {NOP4, NOP4};
+	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
 
 	/* we expect nops at the hook position */
 	if (!expected)
@@ -68,7 +68,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
 				bool enable, bool ra)
 {
 	unsigned int call[2];
-	unsigned int nops[2] = {NOP4, NOP4};
+	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
 
 	if (ra)
 		make_call_ra(hook_pos, target, call);
@@ -97,7 +97,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 		    unsigned long addr)
 {
-	unsigned int nops[2] = {NOP4, NOP4};
+	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
 
 	if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
 		return -EPERM;
diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
index 654ed159c830b3d5e34ac58bf367107066eb73a1..b4c1a6a3fbd28533552036194f27ed206bea305d 100644
--- a/arch/riscv/kernel/jump_label.c
+++ b/arch/riscv/kernel/jump_label.c
@@ -11,8 +11,8 @@
 #include <asm/bug.h>
 #include <asm/cacheflush.h>
 #include <asm/text-patching.h>
+#include <asm/insn-def.h>
 
-#define RISCV_INSN_NOP 0x00000013U
 #define RISCV_INSN_JAL 0x0000006fU
 
 bool arch_jump_label_transform_queue(struct jump_entry *entry,
@@ -33,7 +33,7 @@ bool arch_jump_label_transform_queue(struct jump_entry *entry,
 			(((u32)offset & GENMASK(10,  1)) << (21 -  1)) |
 			(((u32)offset & GENMASK(20, 20)) << (31 - 20));
 	} else {
-		insn = RISCV_INSN_NOP;
+		insn = RISCV_INSN_NOP4;
 	}
 
 	if (early_boot_irqs_disabled) {

-- 
2.43.0
Re: [PATCH v9 1/2] riscv: Move nop definition to insn-def.h
Posted by Andrew Jones 9 months ago 
On Tue, Mar 18, 2025 at 05:38:45PM -0700, Charlie Jenkins wrote:
> We have duplicated the definition of the nop instruction in ftrace.h and
> in jump_label.c. Move this definition into the generic file insn-def.h
> so that they can share the definition with each other and with future
> files.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/asm/ftrace.h   | 1 -
>  arch/riscv/include/asm/insn-def.h | 2 ++
>  arch/riscv/kernel/ftrace.c        | 6 +++---
>  arch/riscv/kernel/jump_label.c    | 4 ++--
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index c4721ce44ca474654b37b3d51bc0a63d46bc1eff..b7f361a50f6445d02a0d88eef5547ee27c1fb52e 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -79,7 +79,6 @@ struct dyn_arch_ftrace {
>  #define AUIPC_RA		(0x00000097)
>  #define JALR_T0			(0x000282e7)
>  #define AUIPC_T0		(0x00000297)
> -#define NOP4			(0x00000013)
>  
>  #define to_jalr_t0(offset)						\
>  	(((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> index 9a913010cdd93cdfdd93f467e7880e20cce0dd2b..0a1fc5134f29da190554c59f8cee3b5374c9aa2d 100644
> --- a/arch/riscv/include/asm/insn-def.h
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -200,4 +200,6 @@
>  #define ZAWRS_WRS_NTO	".4byte 0x00d00073"
>  #define ZAWRS_WRS_STO	".4byte 0x01d00073"
>  
> +#define RISCV_INSN_NOP4	0x00000013U

This should be _AC(0x00000013, U), but since this is the first of its kind
(all other defines are of the form ".4byte ..." -- either explicitly, like
the ones above, or through the INSN_* macros), then it feels like it
either doesn't belong here at all or that we should provide it and a
".4byte ..." version.

Thanks,
drew

> +
>  #endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 3524db5e4fa014a4594465f849d898a030bfb7b8..674dcdfae7a149c339f1e791adb450535f22991b 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -36,7 +36,7 @@ static int ftrace_check_current_call(unsigned long hook_pos,
>  				     unsigned int *expected)
>  {
>  	unsigned int replaced[2];
> -	unsigned int nops[2] = {NOP4, NOP4};
> +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>  
>  	/* we expect nops at the hook position */
>  	if (!expected)
> @@ -68,7 +68,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
>  				bool enable, bool ra)
>  {
>  	unsigned int call[2];
> -	unsigned int nops[2] = {NOP4, NOP4};
> +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>  
>  	if (ra)
>  		make_call_ra(hook_pos, target, call);
> @@ -97,7 +97,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>  		    unsigned long addr)
>  {
> -	unsigned int nops[2] = {NOP4, NOP4};
> +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>  
>  	if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
>  		return -EPERM;
> diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> index 654ed159c830b3d5e34ac58bf367107066eb73a1..b4c1a6a3fbd28533552036194f27ed206bea305d 100644
> --- a/arch/riscv/kernel/jump_label.c
> +++ b/arch/riscv/kernel/jump_label.c
> @@ -11,8 +11,8 @@
>  #include <asm/bug.h>
>  #include <asm/cacheflush.h>
>  #include <asm/text-patching.h>
> +#include <asm/insn-def.h>
>  
> -#define RISCV_INSN_NOP 0x00000013U
>  #define RISCV_INSN_JAL 0x0000006fU
>  
>  bool arch_jump_label_transform_queue(struct jump_entry *entry,
> @@ -33,7 +33,7 @@ bool arch_jump_label_transform_queue(struct jump_entry *entry,
>  			(((u32)offset & GENMASK(10,  1)) << (21 -  1)) |
>  			(((u32)offset & GENMASK(20, 20)) << (31 - 20));
>  	} else {
> -		insn = RISCV_INSN_NOP;
> +		insn = RISCV_INSN_NOP4;
>  	}
>  
>  	if (early_boot_irqs_disabled) {
> 
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v9 1/2] riscv: Move nop definition to insn-def.h
Posted by Charlie Jenkins 9 months ago 
On Wed, Mar 19, 2025 at 04:27:39PM +0100, Andrew Jones wrote:
> On Tue, Mar 18, 2025 at 05:38:45PM -0700, Charlie Jenkins wrote:
> > We have duplicated the definition of the nop instruction in ftrace.h and
> > in jump_label.c. Move this definition into the generic file insn-def.h
> > so that they can share the definition with each other and with future
> > files.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/ftrace.h   | 1 -
> >  arch/riscv/include/asm/insn-def.h | 2 ++
> >  arch/riscv/kernel/ftrace.c        | 6 +++---
> >  arch/riscv/kernel/jump_label.c    | 4 ++--
> >  4 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > index c4721ce44ca474654b37b3d51bc0a63d46bc1eff..b7f361a50f6445d02a0d88eef5547ee27c1fb52e 100644
> > --- a/arch/riscv/include/asm/ftrace.h
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -79,7 +79,6 @@ struct dyn_arch_ftrace {
> >  #define AUIPC_RA		(0x00000097)
> >  #define JALR_T0			(0x000282e7)
> >  #define AUIPC_T0		(0x00000297)
> > -#define NOP4			(0x00000013)
> >  
> >  #define to_jalr_t0(offset)						\
> >  	(((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
> > diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> > index 9a913010cdd93cdfdd93f467e7880e20cce0dd2b..0a1fc5134f29da190554c59f8cee3b5374c9aa2d 100644
> > --- a/arch/riscv/include/asm/insn-def.h
> > +++ b/arch/riscv/include/asm/insn-def.h
> > @@ -200,4 +200,6 @@
> >  #define ZAWRS_WRS_NTO	".4byte 0x00d00073"
> >  #define ZAWRS_WRS_STO	".4byte 0x01d00073"
> >  
> > +#define RISCV_INSN_NOP4	0x00000013U
> 
> This should be _AC(0x00000013, U), but since this is the first of its kind
> (all other defines are of the form ".4byte ..." -- either explicitly, like
> the ones above, or through the INSN_* macros), then it feels like it
> either doesn't belong here at all or that we should provide it and a
> ".4byte ..." version.

Sure, I don't have a strong opinion about this. I was debating adding
the .4byte version also but decided against it because it isn't being
used yet, I will add it.

- Charlie

> 
> Thanks,
> drew
> 
> > +
> >  #endif /* __ASM_INSN_DEF_H */
> > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > index 3524db5e4fa014a4594465f849d898a030bfb7b8..674dcdfae7a149c339f1e791adb450535f22991b 100644
> > --- a/arch/riscv/kernel/ftrace.c
> > +++ b/arch/riscv/kernel/ftrace.c
> > @@ -36,7 +36,7 @@ static int ftrace_check_current_call(unsigned long hook_pos,
> >  				     unsigned int *expected)
> >  {
> >  	unsigned int replaced[2];
> > -	unsigned int nops[2] = {NOP4, NOP4};
> > +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
> >  
> >  	/* we expect nops at the hook position */
> >  	if (!expected)
> > @@ -68,7 +68,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> >  				bool enable, bool ra)
> >  {
> >  	unsigned int call[2];
> > -	unsigned int nops[2] = {NOP4, NOP4};
> > +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
> >  
> >  	if (ra)
> >  		make_call_ra(hook_pos, target, call);
> > @@ -97,7 +97,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> >  int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> >  		    unsigned long addr)
> >  {
> > -	unsigned int nops[2] = {NOP4, NOP4};
> > +	unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
> >  
> >  	if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> >  		return -EPERM;
> > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > index 654ed159c830b3d5e34ac58bf367107066eb73a1..b4c1a6a3fbd28533552036194f27ed206bea305d 100644
> > --- a/arch/riscv/kernel/jump_label.c
> > +++ b/arch/riscv/kernel/jump_label.c
> > @@ -11,8 +11,8 @@
> >  #include <asm/bug.h>
> >  #include <asm/cacheflush.h>
> >  #include <asm/text-patching.h>
> > +#include <asm/insn-def.h>
> >  
> > -#define RISCV_INSN_NOP 0x00000013U
> >  #define RISCV_INSN_JAL 0x0000006fU
> >  
> >  bool arch_jump_label_transform_queue(struct jump_entry *entry,
> > @@ -33,7 +33,7 @@ bool arch_jump_label_transform_queue(struct jump_entry *entry,
> >  			(((u32)offset & GENMASK(10,  1)) << (21 -  1)) |
> >  			(((u32)offset & GENMASK(20, 20)) << (31 - 20));
> >  	} else {
> > -		insn = RISCV_INSN_NOP;
> > +		insn = RISCV_INSN_NOP4;
> >  	}
> >  
> >  	if (early_boot_irqs_disabled) {
> > 
> > -- 
> > 2.43.0
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v9 1/2] riscv: Move nop definition to insn-def.h
Posted by Alexandre Ghiti 9 months ago 
Hi Charlie,

On Wed, Mar 19, 2025 at 1:39 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> We have duplicated the definition of the nop instruction in ftrace.h and
> in jump_label.c. Move this definition into the generic file insn-def.h
> so that they can share the definition with each other and with future
> files.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/asm/ftrace.h   | 1 -
>  arch/riscv/include/asm/insn-def.h | 2 ++
>  arch/riscv/kernel/ftrace.c        | 6 +++---
>  arch/riscv/kernel/jump_label.c    | 4 ++--
>  4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index c4721ce44ca474654b37b3d51bc0a63d46bc1eff..b7f361a50f6445d02a0d88eef5547ee27c1fb52e 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -79,7 +79,6 @@ struct dyn_arch_ftrace {
>  #define AUIPC_RA               (0x00000097)
>  #define JALR_T0                        (0x000282e7)
>  #define AUIPC_T0               (0x00000297)
> -#define NOP4                   (0x00000013)
>
>  #define to_jalr_t0(offset)                                             \
>         (((offset & JALR_OFFSET_MASK) << JALR_SHIFT) | JALR_T0)
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> index 9a913010cdd93cdfdd93f467e7880e20cce0dd2b..0a1fc5134f29da190554c59f8cee3b5374c9aa2d 100644
> --- a/arch/riscv/include/asm/insn-def.h
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -200,4 +200,6 @@
>  #define ZAWRS_WRS_NTO  ".4byte 0x00d00073"
>  #define ZAWRS_WRS_STO  ".4byte 0x01d00073"
>
> +#define RISCV_INSN_NOP4        0x00000013U
> +
>  #endif /* __ASM_INSN_DEF_H */
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 3524db5e4fa014a4594465f849d898a030bfb7b8..674dcdfae7a149c339f1e791adb450535f22991b 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -36,7 +36,7 @@ static int ftrace_check_current_call(unsigned long hook_pos,
>                                      unsigned int *expected)
>  {
>         unsigned int replaced[2];
> -       unsigned int nops[2] = {NOP4, NOP4};
> +       unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>
>         /* we expect nops at the hook position */
>         if (!expected)
> @@ -68,7 +68,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
>                                 bool enable, bool ra)
>  {
>         unsigned int call[2];
> -       unsigned int nops[2] = {NOP4, NOP4};
> +       unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>
>         if (ra)
>                 make_call_ra(hook_pos, target, call);
> @@ -97,7 +97,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>  int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>                     unsigned long addr)
>  {
> -       unsigned int nops[2] = {NOP4, NOP4};
> +       unsigned int nops[2] = {RISCV_INSN_NOP4, RISCV_INSN_NOP4};
>
>         if (patch_insn_write((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
>                 return -EPERM;
> diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> index 654ed159c830b3d5e34ac58bf367107066eb73a1..b4c1a6a3fbd28533552036194f27ed206bea305d 100644
> --- a/arch/riscv/kernel/jump_label.c
> +++ b/arch/riscv/kernel/jump_label.c
> @@ -11,8 +11,8 @@
>  #include <asm/bug.h>
>  #include <asm/cacheflush.h>
>  #include <asm/text-patching.h>
> +#include <asm/insn-def.h>
>
> -#define RISCV_INSN_NOP 0x00000013U
>  #define RISCV_INSN_JAL 0x0000006fU
>
>  bool arch_jump_label_transform_queue(struct jump_entry *entry,
> @@ -33,7 +33,7 @@ bool arch_jump_label_transform_queue(struct jump_entry *entry,
>                         (((u32)offset & GENMASK(10,  1)) << (21 -  1)) |
>                         (((u32)offset & GENMASK(20, 20)) << (31 - 20));
>         } else {
> -               insn = RISCV_INSN_NOP;
> +               insn = RISCV_INSN_NOP4;
>         }
>
>         if (early_boot_irqs_disabled) {
>
> --
> 2.43.0
>

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Tested-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks,

Alex

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.