Re: [PATCH v2 2/2] MIPS: crypto: Add crc32 and crc32c hw accelerated module

[James Hogan <james.hogan@mips.com>]

[-- Attachment #1: Type: text/plain, Size: 6233 bytes --]

(trim cc)

On Wed, Oct 04, 2017 at 05:36:00PM +0200, Ralf Baechle wrote:
> On Wed, Oct 04, 2017 at 12:48:53PM +0200, Marcin Nowakowski wrote:
> > +#define _CRC32(crc, value, size, type)						\
> > +do {										\
> > +	__asm__ __volatile__(							\
> > +	".set	push\n\t"							\
> > +	".set	noat\n\t"							\
> > +	"move	$at, %1\n\t"							\
> > +	"# " #type #size "	%0, $at, %0\n\t"				\
> > +	_ASM_INSN_IF_MIPS(0x7c00000f | (2 << 16) | (1 << 21) | (%2 << 6) | (%3 << 8))	\
> > +	_ASM_INSN32_IF_MM(0x00000030 | (1 << 16) | (2 << 21) | (%2 << 14) | (%3 << 3))	\
> > +	".set	pop"							\
> > +	: "+r" (crc)								\
> > +	: "r" (value), "i" (size), "i" (type)					\
> > +);										\
> > +} while(0)
> > +
> > +#define CRC_REGISTER __asm__("$2")
> > +#endif	/* !TOOLCHAIN_SUPPORTS_CRC */
> 
> Over there years we've added so many inlines for instructions not yet
> supported by gas and the simply approach always requires extra move
> instructions.  So I finally cooked up something better which only relies
> on things that are in gas for as long as I can remember.  This illustrates
> it:
> 
> 	.macro	parse var r
> 	.ifc	\r, $0
> \var	=	0
> 	.endif
> 	.ifc	\r, $1
> \var	=	1
> 	.endif
> 	.ifc	\r, $2
> \var	=	2
> 	.endif
> 	.ifc	\r, $3
> \var	=	3
> 	.endif
> 	.endm
> 
> 	.macro	definsn opcode r1 r2 r3
> 	parse	__definsn_r1 \r1
> 	parse	__definsn_r2 \r2
> 	parse	__definsn_r3 \r3
> 	.word	\opcode | (__definsn_r1 << 16) | (__definsn_r2 << 20) | (__definsn_r3 << 24)
> 	.endm
> 
> 	.macro	foo r1, r2, r3
> 	definsn	42, \r1, \r2, \r3
> 	.endm
> 
> 	foo	$1, $2, $3
> 	foo	$3, $0, $1
> 
> The advantages are obvious, the hypothetical instruction foo can be used
> just like it was actually supported by gas and no bloat by move
> instructions to shuffle operands in and results out.  Which for some
> very convoluted cases may also save you from an ICE.
> 
> Above skeleton still needs a little polish, either by extending the
> parsing of register numbers to all 32 registers or by implementing that
> using another loop which probably would be implemented using a recursive
> gas macro.

Very nice, e.g.

diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
index 6b1f1ad0542c..3a16fad43efb 100644
--- a/arch/mips/include/asm/mipsregs.h
+++ b/arch/mips/include/asm/mipsregs.h
@@ -1180,6 +1180,31 @@ static inline int mm_insn_16bit(u16 insn)
 #define _ASM_INSN_IF_MIPS(_enc)
 #endif
 
+/*
+ * Helper assembly macro for decoding a register name.
+ */
+#define _IFC_REG(n)			\
+	".ifc	\\r, $"#n"\n\t"		\
+	"\\var	="#n"\n\t"		\
+	".endif\n\t"
+
+__asm__(".macro	parse var r\n\t"
+	"\\var	= -1\n\t"
+	_IFC_REG( 0) _IFC_REG( 1) _IFC_REG( 2) _IFC_REG( 3)
+	_IFC_REG( 4) _IFC_REG( 5) _IFC_REG( 6) _IFC_REG( 7)
+	_IFC_REG( 8) _IFC_REG( 9) _IFC_REG(10) _IFC_REG(11)
+	_IFC_REG(12) _IFC_REG(13) _IFC_REG(14) _IFC_REG(15)
+	_IFC_REG(16) _IFC_REG(17) _IFC_REG(18) _IFC_REG(19)
+	_IFC_REG(20) _IFC_REG(21) _IFC_REG(22) _IFC_REG(23)
+	_IFC_REG(24) _IFC_REG(25) _IFC_REG(26) _IFC_REG(27)
+	_IFC_REG(28) _IFC_REG(29) _IFC_REG(30) _IFC_REG(31)
+	".iflt	\\var\n\t"
+	".error \"Unable to parse register name \\r\"\n\t"
+	".endif\n\t"
+	".endm");
+
+#undef _IFC_REG
+
 /*
  * TLB Invalidate Flush
  */
@@ -1412,10 +1437,10 @@ do {									\
 	"	.set	push					\n"	\
 	"	.set	noat					\n"	\
 	"	.set	mips32r2				\n"	\
-	"	# mfhc0 $1, %1					\n"	\
-	_ASM_INSN_IF_MIPS(0x40410000 | ((%1 & 0x1f) << 11))		\
-	_ASM_INSN32_IF_MM(0x002000f4 | ((%1 & 0x1f) << 16))		\
-	"	move	%0, $1					\n"	\
+	"	# mfhc0 %0, %1					\n"	\
+	"	parse	__dst, %0				\n"	\
+	_ASM_INSN_IF_MIPS(0x40400000 | (__dst << 16) | ((%1 & 0x1f) << 11)) \
+	_ASM_INSN32_IF_MM(0x000000f4 | (__dst << 21) | ((%1 & 0x1f) << 16)) \
 	"	.set	pop					\n"	\
 	: "=r" (__res)							\
 	: "i" (source));						\

gives us this in arch/mips/lib/dump_tlb.o:

- 430:	40411000 	mfhc0	at,c0_entrylo0
- 434:	00203825 	move	a3,at
+ 430:	40471000 	mfhc0	a3,c0_entrylo0

And we can easily enough simplify a lot of mostly duplicated code by
doing something like this (can't do it with mfhc0 as we don't yet have a
case that uses the assembler):

@@ -1855,14 +1855,25 @@ do {									\
  * Macros to access the guest system control coprocessor
  */
 
-#ifdef TOOLCHAIN_SUPPORTS_VIRT
+#ifndef TOOLCHAIN_SUPPORTS_VIRT
+__asm__(".macro	mfgc0 rt, rs, sel\n\t"
+	"parse __rt, \\rt\n\t"
+	"parse __rs, \\rs\n\t"
+	"# mfgc0\t\\rt, \\rs, %2\n\t"
+	_ASM_INSN_IF_MIPS(0x40600000 | __rt << 16 | __rs << 11 | \\sel)
+	_ASM_INSN32_IF_MM(0x000004fc | __rt << 21 | __rs << 16 | \\sel << 11)
+	".endm");
+#define _ASM_SET_VIRT ""
+#else
+#define _ASM_SET_VIRT ".set\tvirt\n\t"
+#endif
 
 #define __read_32bit_gc0_register(source, sel)				\
 ({ int __res;								\
 	__asm__ __volatile__(						\
 		".set\tpush\n\t"					\
 		".set\tmips32r2\n\t"					\
-		".set\tvirt\n\t"					\
+		_ASM_SET_VIRT						\
 		"mfgc0\t%0, $%1, %2\n\t"				\
 		".set\tpop"						\
 		: "=r" (__res)						\
@@ -1870,6 +1881,8 @@ do {									\
 	__res;								\
 })
 
+#ifdef TOOLCHAIN_SUPPORTS_VIRT
+
 #define __read_64bit_gc0_register(source, sel)				\
 ({ unsigned long long __res;						\
 	__asm__ __volatile__(						\
@@ -1909,21 +1922,6 @@ do {									\
 
 #else	/* TOOLCHAIN_SUPPORTS_VIRT */
 
-#define __read_32bit_gc0_register(source, sel)				\
-({ int __res;								\
-	__asm__ __volatile__(						\
-		".set\tpush\n\t"					\
-		".set\tnoat\n\t"					\
-		"# mfgc0\t$1, $%1, %2\n\t"				\
-		_ASM_INSN_IF_MIPS(0x40610000 | %1 << 11 | %2)		\
-		_ASM_INSN32_IF_MM(0x002004fc | %1 << 16 | %2 << 11)	\
-		"move\t%0, $1\n\t"					\
-		".set\tpop"						\
-		: "=r" (__res)						\
-		: "i" (source), "i" (sel));				\
-	__res;								\
-})
-
 #define __read_64bit_gc0_register(source, sel)				\
 ({ unsigned long long __res;						\
 	__asm__ __volatile__(						\

A worthwhile cleanup IMO to fix all cases like this to use that parsing
macro, though it'll take a bit of care.

The use of __asm__ in global scope to define the macros to be accessible
from inline asm is slightly hacky.

Any comments / objections?

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]