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

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

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

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 <ubizjak@gmail.com>
---
 arch/x86/include/asm/arch_hweight.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h
index 34a10b2d5b73..d668e411bd32 100644
--- a/arch/x86/include/asm/arch_hweight.h
+++ b/arch/x86/include/asm/arch_hweight.h
@@ -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));
 
-- 
2.17.2

Re: [PATCH] x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

[Borislav Petkov]

On Sun, Oct 14, 2018 at 08:35:10PM +0200, Uros Bizjak wrote:
> 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...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

[Uros Bizjak]

On Sun, Oct 14, 2018 at 8:47 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Oct 14, 2018 at 08:35:10PM +0200, Uros Bizjak wrote:
> > 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.

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

Re: [PATCH] x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

[Borislav Petkov]

On Sun, Oct 14, 2018 at 09:15:00PM +0200, Uros Bizjak wrote:
> The ChangeLog says "real INSTRUCTION mnemonics", e.g. POPCNTQ and POPCNTL.

Right, INSTRUCTION.

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

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

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86: Use assembly instruction mnemonics instead of .byte streams in arch_hweight.h

[Uros Bizjak]

On Sun, Oct 14, 2018 at 10:02 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Oct 14, 2018 at 09:15:00PM +0200, Uros Bizjak wrote:
> > The ChangeLog says "real INSTRUCTION mnemonics", e.g. POPCNTQ and POPCNTL.
>
> Right, INSTRUCTION.
>
> > 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'.
>
> > 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.