Skip to contentSkip to conversation

x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

PATCH Superseded Uros Bizjak 2 participants 5 messages Oct 14, 2018 x86

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Uros Bizjak

author
#0 lore raw

PATCH 1/1 x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

Recently the minimum required version of binutils was changed to 2.20, which supports popcnt instruction mnemonics. The patch removes all .byte #defines and uses real instruction mnemonics instead.

Tested by building x86_64 and i386 version of the kernel and comparing objdump dumps of the pathced and unpatched vmlinux.o. They were the same.

Signed-off-by Uros Bizjak <[email protected]>
---
 arch/x86/include/asm/arch_hweight.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
arch/x86/include/asm/arch_hweight.h +4 -8
index 34a10b2d5b73..d668e411bd32 100644
@@ -5,15 +5,9 @@
 #include <asm/cpufeatures.h>
 
 #ifdef CONFIG_64BIT
-/* popcnt %edi, %eax */
-#define POPCNT32 ".byte 0xf3,0x0f,0xb8,0xc7"
-/* popcnt %rdi, %rax */
-#define POPCNT64 ".byte 0xf3,0x48,0x0f,0xb8,0xc7"
 #define REG_IN "D"
 #define REG_OUT "a"
 #else
-/* popcnt %eax, %eax */
-#define POPCNT32 ".byte 0xf3,0x0f,0xb8,0xc0"
 #define REG_IN "a"
 #define REG_OUT "a"
 #endif

@@ -24,7 +18,8 @@ static __always_inline unsigned int __arch_hweight32(unsigned int w)
 {
 	unsigned int res;
 
-	asm (ALTERNATIVE("call __sw_hweight32", POPCNT32, X86_FEATURE_POPCNT)
+	asm (ALTERNATIVE("call __sw_hweight32",
+			 "popcntl %1, %0", X86_FEATURE_POPCNT)
 			 : "="REG_OUT (res)
 			 : REG_IN (w));
 

@@ -52,7 +47,8 @@ static __always_inline unsigned long __arch_hweight64(__u64 w)
 {
 	unsigned long res;
 
-	asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT)
+	asm (ALTERNATIVE("call __sw_hweight64",
+			 "popcntq %1, %0", X86_FEATURE_POPCNT)
 			 : "="REG_OUT (res)
 			 : REG_IN (w));
Signature · 1 line
2.17.2
maintainer
#1 lore raw
Quoted 3 lines · Uros Bizjak

Recently the minimum required version of binutils was changed to 2.20, which supports popcnt instruction mnemonics. The patch removes all .byte #defines and uses real instruction mnemonics instead.

What is "real insertion mnemonics" ?

To me it looks like this patch replaces our defines with binutils' defines and frankly, if it ain't broke, why fix it...

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#2 lore raw
Quoted 7 lines · Borislav Petkov
Quoted 3 lines · Uros Bizjak

Recently the minimum required version of binutils was changed to 2.20, which supports popcnt instruction mnemonics. The patch removes all .byte #defines and uses real instruction mnemonics instead.

What is "real insertion mnemonics" ?

The ChangeLog says "real INSTRUCTION mnemonics", e.g. POPCNTQ and POPCNTL.

Quoted 2 lines

To me it looks like this patch replaces our defines with binutils' defines and frankly, if it ain't broke, why fix it...

The compiler will generate the register name with the correct implied width (e.g. %rax for long, %eax for int), so the assembler will be able to cross check if operands fit the instruction (this issue happened in KVM, see [1]). And there will be a couple of ugly #defines less.

[1] https://www.spinics.net/lists/kvm/msg176184.html

Uros.

maintainer
#3 lore raw
Quoted 1 line · Uros Bizjak

The ChangeLog says "real INSTRUCTION mnemonics", e.g. POPCNTQ and POPCNTL.

Right, INSTRUCTION.

Quoted 3 lines

The compiler will generate the register name with the correct implied width (e.g. %rax for long, %eax for int), so the assembler will be able to cross check if operands fit the instruction

The __arch_hweightXX functions already enforce the proper type and the inline asm() operands already place the arguments in the proper registers where the instruction encoding expects them.

So if you're going to relax this, then you could relax the inline asm operand specifications too. I say you "could" because then you need to fix arch/x86/lib/hweight.S too, which would be at least ugly. So I think we're stuck with %xDI/xAX and %xAX as operands, where 'x' is either 'r' or 'e'.

Quoted 1 line

And there will be a couple of ugly #defines less.

That's the only advantage of this change AFAICT. How about you reflect that in your commit message?

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#4 lore raw
Quoted 20 lines · Borislav Petkov
Quoted 1 line · Uros Bizjak

The ChangeLog says "real INSTRUCTION mnemonics", e.g. POPCNTQ and POPCNTL.

Right, INSTRUCTION.

Quoted 3 lines

The compiler will generate the register name with the correct implied width (e.g. %rax for long, %eax for int), so the assembler will be able to cross check if operands fit the instruction

The __arch_hweightXX functions already enforce the proper type and the inline asm() operands already place the arguments in the proper registers where the instruction encoding expects them.

So if you're going to relax this, then you could relax the inline asm operand specifications too. I say you "could" because then you need to fix arch/x86/lib/hweight.S too, which would be at least ugly. So I think we're stuck with %xDI/xAX and %xAX as operands, where 'x' is either 'r' or 'e'.

Quoted 4 lines
Quoted 1 line

And there will be a couple of ugly #defines less.

That's the only advantage of this change AFAICT. How about you reflect that in your commit message?

No problem, will send v2 with amended message.

Uros.