[PATCH] riscv: asm: use .insn for making custom instructioons

[PATCH] riscv: asm: use .insn for making custom instructioons

[Ben Dooks]

Using .word breaks with big endian builds, making something which
is not a valid or worse an instruction or pair that does something
which is not intended.

It would seem sensible to add an ASM_INSN() wrapper for anyone to
use for hand assembly of instructions.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/riscv/include/asm/asm.h      | 6 ++++++
 arch/riscv/include/asm/insn-def.h | 8 ++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h
index 8bd2a11382a3..c92f0ff51ffa 100644
--- a/arch/riscv/include/asm/asm.h
+++ b/arch/riscv/include/asm/asm.h
@@ -12,6 +12,12 @@
 #define __ASM_STR(x)	#x
 #endif
 
+#ifndef CONFIG_AS_HAS_INSN
+#define ASM_INSN(__x) ".4byte " __x
+#else
+#define ASM_INSN(__x) ".insn " __x
+#endif
+
 #if __riscv_xlen == 64
 #define __REG_SEL(a, b)	__ASM_STR(a)
 #elif __riscv_xlen == 32
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index c9cfcea52cbb..35b5024db0fc 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -256,10 +256,10 @@
 	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3),		\
 	       SIMM12((offset) & 0xfe0), RS1(base))
 
-#define RISCV_PAUSE	".4byte 0x100000f"
-#define ZAWRS_WRS_NTO	".4byte 0x00d00073"
-#define ZAWRS_WRS_STO	".4byte 0x01d00073"
-#define RISCV_NOP4	".4byte 0x00000013"
+#define RISCV_PAUSE	ASM_INSN("0x100000f")
+#define ZAWRS_WRS_NTO	ASM_INSN("0x00d00073")
+#define ZAWRS_WRS_STO	ASM_INSN("0x01d00073")
+#define RISCV_NOP4	ASM_INSN("0x00000013")
 
 #define RISCV_INSN_NOP4	_AC(0x00000013, U)
 
-- 
2.37.2.352.g3c44437643


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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Paul Walmsley]

Hi Ben,

On Fri, 17 Oct 2025, Ben Dooks wrote:

> Using .word breaks with big endian builds, making something which
> is not a valid or worse an instruction or pair that does something
> which is not intended.
> 
> It would seem sensible to add an ASM_INSN() wrapper for anyone to
> use for hand assembly of instructions.

Looking at the GNU as documentation for RISC-V:

  https://sourceware.org/binutils/docs/as/RISC_002dV_002dDirectives.html

it seems that RISC-V .insn supports several argument formats that .4byte 
wouldn't.  So it seems this is only applicable for opcodes specified as 
hex bytes?  Might be worth adding that to the macro name in some form.  

Also, looks like this is missing the opcodes added in commit 
1d4ce63e338fc.  If you update this one, could you please add those as 
well?

thanks


- Paul

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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Andrew Jones]

On Fri, Oct 17, 2025 at 05:55:27PM +0100, Ben Dooks wrote:
> Using .word breaks with big endian builds, making something which
> is not a valid or worse an instruction or pair that does something
> which is not intended.

The motivation is no longer for big endian and, since big endian
doesn't require AS_HAS_INSN, is never really could be without adding
that dependency. The commit message should be changed to point out
that we want to use .insn when possible because it provides validation
and allows mapping symbols to identify the words as instructions.

> 
> It would seem sensible to add an ASM_INSN() wrapper for anyone to
> use for hand assembly of instructions.

This sentence isn't necessary.

> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  arch/riscv/include/asm/asm.h      | 6 ++++++
>  arch/riscv/include/asm/insn-def.h | 8 ++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h
> index 8bd2a11382a3..c92f0ff51ffa 100644
> --- a/arch/riscv/include/asm/asm.h
> +++ b/arch/riscv/include/asm/asm.h
> @@ -12,6 +12,12 @@
>  #define __ASM_STR(x)	#x
>  #endif
>  
> +#ifndef CONFIG_AS_HAS_INSN
> +#define ASM_INSN(__x) ".4byte " __x
> +#else
> +#define ASM_INSN(__x) ".insn " __x
> +#endif
> +
>  #if __riscv_xlen == 64
>  #define __REG_SEL(a, b)	__ASM_STR(a)
>  #elif __riscv_xlen == 32
> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
> index c9cfcea52cbb..35b5024db0fc 100644
> --- a/arch/riscv/include/asm/insn-def.h
> +++ b/arch/riscv/include/asm/insn-def.h
> @@ -256,10 +256,10 @@
>  	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3),		\
>  	       SIMM12((offset) & 0xfe0), RS1(base))
>  
> -#define RISCV_PAUSE	".4byte 0x100000f"
> -#define ZAWRS_WRS_NTO	".4byte 0x00d00073"
> -#define ZAWRS_WRS_STO	".4byte 0x01d00073"
> -#define RISCV_NOP4	".4byte 0x00000013"
> +#define RISCV_PAUSE	ASM_INSN("0x100000f")
> +#define ZAWRS_WRS_NTO	ASM_INSN("0x00d00073")
> +#define ZAWRS_WRS_STO	ASM_INSN("0x01d00073")
> +#define RISCV_NOP4	ASM_INSN("0x00000013")
>  
>  #define RISCV_INSN_NOP4	_AC(0x00000013, U)

Other than the commit message changes, the patch looks good.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Maciej W. Rozycki]

On Fri, 17 Oct 2025, Ben Dooks wrote:

> diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h
> index 8bd2a11382a3..c92f0ff51ffa 100644
> --- a/arch/riscv/include/asm/asm.h
> +++ b/arch/riscv/include/asm/asm.h
> @@ -12,6 +12,12 @@
>  #define __ASM_STR(x)	#x
>  #endif
>  
> +#ifndef CONFIG_AS_HAS_INSN
> +#define ASM_INSN(__x) ".4byte " __x
> +#else
> +#define ASM_INSN(__x) ".insn " __x
> +#endif

 FWIW writing code such that double negation applies to the else clause 
makes conditionals harder to parse by humans (it's !!CONFIG_AS_HAS_INSN 
effectively here).  Would you mind rewriting it as:

#ifdef CONFIG_AS_HAS_INSN
#define ASM_INSN(__x) ".insn " __x
#else
#define ASM_INSN(__x) ".4byte " __x
#endif

or has there been a particular reason you chose the proposed form?

  Maciej

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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Ben Dooks]

On 17/10/2025 19:25, Paul Walmsley wrote:
> Hi Ben,
> 
> On Fri, 17 Oct 2025, Ben Dooks wrote:
> 
>> Using .word breaks with big endian builds, making something which
>> is not a valid or worse an instruction or pair that does something
>> which is not intended.
>>
>> It would seem sensible to add an ASM_INSN() wrapper for anyone to
>> use for hand assembly of instructions.
> 
> Looking at the GNU as documentation for RISC-V:
> 
>    https://sourceware.org/binutils/docs/as/RISC_002dV_002dDirectives.html
> 
> it seems that RISC-V .insn supports several argument formats that .4byte
> wouldn't.  So it seems this is only applicable for opcodes specified as
> hex bytes?  Might be worth adding that to the macro name in some form.

Changed it to ASM_INSN_I


> Also, looks like this is missing the opcodes added in commit
> 1d4ce63e338fc.  If you update this one, could you please add those as
> well?

I'll go change those too.

Thank you for the review.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Ben Dooks]

On 18/10/2025 05:50, Maciej W. Rozycki wrote:
> On Fri, 17 Oct 2025, Ben Dooks wrote:
> 
>> diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h
>> index 8bd2a11382a3..c92f0ff51ffa 100644
>> --- a/arch/riscv/include/asm/asm.h
>> +++ b/arch/riscv/include/asm/asm.h
>> @@ -12,6 +12,12 @@
>>   #define __ASM_STR(x)	#x
>>   #endif
>>   
>> +#ifndef CONFIG_AS_HAS_INSN
>> +#define ASM_INSN(__x) ".4byte " __x
>> +#else
>> +#define ASM_INSN(__x) ".insn " __x
>> +#endif
> 
>   FWIW writing code such that double negation applies to the else clause
> makes conditionals harder to parse by humans (it's !!CONFIG_AS_HAS_INSN
> effectively here).  Would you mind rewriting it as:
> 
> #ifdef CONFIG_AS_HAS_INSN
> #define ASM_INSN(__x) ".insn " __x
> #else
> #define ASM_INSN(__x) ".4byte " __x
> #endif
> 
> or has there been a particular reason you chose the proposed form?

Thanks, no idea why it was this way, fixed for v2.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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

Re: [PATCH] riscv: asm: use .insn for making custom instructioons

[Ben Dooks]

On 17/10/2025 19:27, Andrew Jones wrote:
> On Fri, Oct 17, 2025 at 05:55:27PM +0100, Ben Dooks wrote:
>> Using .word breaks with big endian builds, making something which
>> is not a valid or worse an instruction or pair that does something
>> which is not intended.
> 
> The motivation is no longer for big endian and, since big endian
> doesn't require AS_HAS_INSN, is never really could be without adding
> that dependency. The commit message should be changed to point out
> that we want to use .insn when possible because it provides validation
> and allows mapping symbols to identify the words as instructions.
> 
>>
>> It would seem sensible to add an ASM_INSN() wrapper for anyone to
>> use for hand assembly of instructions.
> 
> This sentence isn't necessary.
> 
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   arch/riscv/include/asm/asm.h      | 6 ++++++
>>   arch/riscv/include/asm/insn-def.h | 8 ++++----
>>   2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h
>> index 8bd2a11382a3..c92f0ff51ffa 100644
>> --- a/arch/riscv/include/asm/asm.h
>> +++ b/arch/riscv/include/asm/asm.h
>> @@ -12,6 +12,12 @@
>>   #define __ASM_STR(x)	#x
>>   #endif
>>   
>> +#ifndef CONFIG_AS_HAS_INSN
>> +#define ASM_INSN(__x) ".4byte " __x
>> +#else
>> +#define ASM_INSN(__x) ".insn " __x
>> +#endif
>> +
>>   #if __riscv_xlen == 64
>>   #define __REG_SEL(a, b)	__ASM_STR(a)
>>   #elif __riscv_xlen == 32
>> diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
>> index c9cfcea52cbb..35b5024db0fc 100644
>> --- a/arch/riscv/include/asm/insn-def.h
>> +++ b/arch/riscv/include/asm/insn-def.h
>> @@ -256,10 +256,10 @@
>>   	INSN_S(OPCODE_OP_IMM, FUNC3(6), __RS2(3),		\
>>   	       SIMM12((offset) & 0xfe0), RS1(base))
>>   
>> -#define RISCV_PAUSE	".4byte 0x100000f"
>> -#define ZAWRS_WRS_NTO	".4byte 0x00d00073"
>> -#define ZAWRS_WRS_STO	".4byte 0x01d00073"
>> -#define RISCV_NOP4	".4byte 0x00000013"
>> +#define RISCV_PAUSE	ASM_INSN("0x100000f")
>> +#define ZAWRS_WRS_NTO	ASM_INSN("0x00d00073")
>> +#define ZAWRS_WRS_STO	ASM_INSN("0x01d00073")
>> +#define RISCV_NOP4	ASM_INSN("0x00000013")
>>   
>>   #define RISCV_INSN_NOP4	_AC(0x00000013, U)
> 
> Other than the commit message changes, the patch looks good.
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Thanks,
> drew


Re-worded and add the review.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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