[PATCH] x86: Use INVPCID mnemonic in invpcid.h

[PATCH] x86: Use INVPCID mnemonic in invpcid.h

[Uros Bizjak]

Current minimum required version of binutils is 2.23,
which supports INVPCID instruction mnemonic.

Replace the byte-wise specification of INVPCID with
this proper mnemonic.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/invpcid.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
index 989cfa86de85..23749bbca0ad 100644
--- a/arch/x86/include/asm/invpcid.h
+++ b/arch/x86/include/asm/invpcid.h
@@ -12,12 +12,9 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
 	 * stale TLB entries and, especially if we're flushing global
 	 * mappings, we don't want the compiler to reorder any subsequent
 	 * memory accesses before the TLB flush.
-	 *
-	 * The hex opcode is invpcid (%ecx), %eax in 32-bit mode and
-	 * invpcid (%rcx), %rax in long mode.
 	 */
-	asm volatile (".byte 0x66, 0x0f, 0x38, 0x82, 0x01"
-		      : : "m" (desc), "a" (type), "c" (&desc) : "memory");
+	asm volatile ("invpcid %1, %0"
+		      : : "r" (type), "m" (desc) : "memory");
 }
 
 #define INVPCID_TYPE_INDIV_ADDR		0
-- 
2.25.4

Re: [PATCH] x86: Use INVPCID mnemonic in invpcid.h

[Peter Zijlstra]

On Fri, May 08, 2020 at 11:22:47AM +0200, Uros Bizjak wrote:
> Current minimum required version of binutils is 2.23,
> which supports INVPCID instruction mnemonic.
> 
> Replace the byte-wise specification of INVPCID with
> this proper mnemonic.

Excellent, thanks for doing these cleanups.

(also for the rand one):

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH] x86: Use INVPCID mnemonic in invpcid.h

[H. Peter Anvin]

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>

On 2020-05-08 02:22, Uros Bizjak wrote:
> Current minimum required version of binutils is 2.23,
> which supports INVPCID instruction mnemonic.
> 
> Replace the byte-wise specification of INVPCID with
> this proper mnemonic.
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/invpcid.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
> index 989cfa86de85..23749bbca0ad 100644
> --- a/arch/x86/include/asm/invpcid.h
> +++ b/arch/x86/include/asm/invpcid.h
> @@ -12,12 +12,9 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
>  	 * stale TLB entries and, especially if we're flushing global
>  	 * mappings, we don't want the compiler to reorder any subsequent
>  	 * memory accesses before the TLB flush.
> -	 *
> -	 * The hex opcode is invpcid (%ecx), %eax in 32-bit mode and
> -	 * invpcid (%rcx), %rax in long mode.
>  	 */
> -	asm volatile (".byte 0x66, 0x0f, 0x38, 0x82, 0x01"
> -		      : : "m" (desc), "a" (type), "c" (&desc) : "memory");
> +	asm volatile ("invpcid %1, %0"
> +		      : : "r" (type), "m" (desc) : "memory");
>  }
>  
>  #define INVPCID_TYPE_INDIV_ADDR		0
> 

Re: [PATCH] x86: Use INVPCID mnemonic in invpcid.h

[Borislav Petkov]

On Fri, May 08, 2020 at 11:22:47AM +0200, Uros Bizjak wrote:
> Current minimum required version of binutils is 2.23,
> which supports INVPCID instruction mnemonic.
> 
> Replace the byte-wise specification of INVPCID with
> this proper mnemonic.
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/invpcid.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
> index 989cfa86de85..23749bbca0ad 100644
> --- a/arch/x86/include/asm/invpcid.h
> +++ b/arch/x86/include/asm/invpcid.h
> @@ -12,12 +12,9 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
>  	 * stale TLB entries and, especially if we're flushing global
>  	 * mappings, we don't want the compiler to reorder any subsequent
>  	 * memory accesses before the TLB flush.
> -	 *
> -	 * The hex opcode is invpcid (%ecx), %eax in 32-bit mode and
> -	 * invpcid (%rcx), %rax in long mode.
>  	 */
> -	asm volatile (".byte 0x66, 0x0f, 0x38, 0x82, 0x01"
> -		      : : "m" (desc), "a" (type), "c" (&desc) : "memory");
> +	asm volatile ("invpcid %1, %0"
> +		      : : "r" (type), "m" (desc) : "memory");

How about I make it even more readable when applying?

        asm volatile("invpcid %[desc], %[type]"
                     :: [desc] "m" (desc), [type] "r" (type) : "memory");

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/cpu] x86/cpu: Use INVPCID mnemonic in invpcid.h

[tip-bot2 for Uros Bizjak]

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     7e32a9dac9926241d56851e1517c9391d39fb48e
Gitweb:        https://git.kernel.org/tip/7e32a9dac9926241d56851e1517c9391d39fb48e
Author:        Uros Bizjak <ubizjak@gmail.com>
AuthorDate:    Fri, 08 May 2020 11:22:47 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 12 May 2020 16:05:30 +02:00

x86/cpu: Use INVPCID mnemonic in invpcid.h

The current minimum required version of binutils is 2.23, which supports
the INVPCID instruction mnemonic. Replace the byte-wise specification of
INVPCID with the proper mnemonic.

 [ bp: Add symbolic operand names for increased readability and flip
   their order like the insn expects them for the AT&T syntax. ]

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200508092247.132147-1-ubizjak@gmail.com

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/invpcid.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
index 989cfa8..734482a 100644
--- a/arch/x86/include/asm/invpcid.h
+++ b/arch/x86/include/asm/invpcid.h
@@ -12,12 +12,9 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
 	 * stale TLB entries and, especially if we're flushing global
 	 * mappings, we don't want the compiler to reorder any subsequent
 	 * memory accesses before the TLB flush.
-	 *
-	 * The hex opcode is invpcid (%ecx), %eax in 32-bit mode and
-	 * invpcid (%rcx), %rax in long mode.
 	 */
-	asm volatile (".byte 0x66, 0x0f, 0x38, 0x82, 0x01"
-		      : : "m" (desc), "a" (type), "c" (&desc) : "memory");
+	asm volatile("invpcid %[desc], %[type]"
+		     :: [desc] "m" (desc), [type] "r" (type) : "memory");
 }
 
 #define INVPCID_TYPE_INDIV_ADDR		0

Re: [tip: x86/cpu] x86/cpu: Use INVPCID mnemonic in invpcid.h

[Uros Bizjak]

On Tue, May 12, 2020 at 4:10 PM tip-bot2 for Uros Bizjak
<tip-bot2@linutronix.de> wrote:
>
> The following commit has been merged into the x86/cpu branch of tip:
>
> Commit-ID:     7e32a9dac9926241d56851e1517c9391d39fb48e
> Gitweb:        https://git.kernel.org/tip/7e32a9dac9926241d56851e1517c9391d39fb48e
> Author:        Uros Bizjak <ubizjak@gmail.com>
> AuthorDate:    Fri, 08 May 2020 11:22:47 +02:00
> Committer:     Borislav Petkov <bp@suse.de>
> CommitterDate: Tue, 12 May 2020 16:05:30 +02:00
>
> x86/cpu: Use INVPCID mnemonic in invpcid.h
>
> The current minimum required version of binutils is 2.23, which supports
> the INVPCID instruction mnemonic. Replace the byte-wise specification of
> INVPCID with the proper mnemonic.
>
>  [ bp: Add symbolic operand names for increased readability and flip
>    their order like the insn expects them for the AT&T syntax. ]

Actually, the order was correct for AT&T syntax in the original patch.

The insn template for AT&T syntax goes:

insn arg2, arg1, arg0

where rightmost arguments are output operands.

The operands in asm template go

asm ("insn template" : output0, output1 : input0, input1 : clobbers)

so, in effect:

asm ("insn template" : arg0, arg1 : arg2, arg3: clobbers)

As you can see, the operand order in insn tempate is reversed for AT&T
syntax. I didn't notice the reversal of operands in your improvement.

Uros.


Uros.

Re: [tip: x86/cpu] x86/cpu: Use INVPCID mnemonic in invpcid.h

[Borislav Petkov]

On Tue, May 12, 2020 at 04:26:37PM +0200, Uros Bizjak wrote:
> Actually, the order was correct for AT&T syntax in the original patch.
> 
> The insn template for AT&T syntax goes:
> 
> insn arg2, arg1, arg0
> 
> where rightmost arguments are output operands.
> 
> The operands in asm template go
> 
> asm ("insn template" : output0, output1 : input0, input1 : clobbers)
> 
> so, in effect:
> 
> asm ("insn template" : arg0, arg1 : arg2, arg3: clobbers)
> 
> As you can see, the operand order in insn tempate is reversed for AT&T
> syntax. I didn't notice the reversal of operands in your improvement.

Your version had:

+       asm volatile ("invpcid %1, %0"
+                     : : "r" (type), "m" (desc) : "memory");

with "type" being the 0th operand and "desc" being the 1st operand in
the input operands list.

The order of the operands after the "invpcid" mnemonic are the other way
around though: you first have %1 which is "desc" and then %0 which is
the type.

I simply swapped the arguments order in the input operands list, after
the second ':'

+       asm volatile("invpcid %[desc], %[type]"
+                    :: [desc] "m" (desc), [type] "r" (type) : "memory");

so that "desc" comes first and "type" second when reading from
left-to-right in both

1. *after* the "invpcid" mnemonic and
2. in the input operands list, after the second ':'.

And since I'm using the symbolic operand names, then the order just
works because looking at a before-and-after thing doesn't show any
opcode differences:

$ diff -suprN /tmp/before /tmp/after
Files /tmp/before and /tmp/after are identical

$ cat /tmp/before
ffffffff81058392:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105c542:	66 44 0f 38 82 04 24 	invpcid (%rsp),%r8
ffffffff8105c5b4:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105ccd2:	66 44 0f 38 82 14 24 	invpcid (%rsp),%r10
ffffffff8105d6cf:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105d7ef:	66 0f 38 82 1c 24    	invpcid (%rsp),%rbx
ffffffff8105e4db:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8106067b:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8169547d:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82406698 <x86_noinvpcid_setup>:
ffffffff824066a3:	75 27                	jne    ffffffff824066cc <x86_noinvpcid_setup+0x34>
ffffffff824066b4:	73 16                	jae    ffffffff824066cc <x86_noinvpcid_setup+0x34>
ffffffff824124db:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82412e34:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82415158:	66 0f 38 82 44 24 10 	invpcid 0x10(%rsp),%rax

$ cat /tmp/after
ffffffff81058392:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105c542:	66 44 0f 38 82 04 24 	invpcid (%rsp),%r8
ffffffff8105c5b4:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105ccd2:	66 44 0f 38 82 14 24 	invpcid (%rsp),%r10
ffffffff8105d6cf:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8105d7ef:	66 0f 38 82 1c 24    	invpcid (%rsp),%rbx
ffffffff8105e4db:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8106067b:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff8169547d:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82406698 <x86_noinvpcid_setup>:
ffffffff824066a3:	75 27                	jne    ffffffff824066cc <x86_noinvpcid_setup+0x34>
ffffffff824066b4:	73 16                	jae    ffffffff824066cc <x86_noinvpcid_setup+0x34>
ffffffff824124db:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82412e34:	66 0f 38 82 04 24    	invpcid (%rsp),%rax
ffffffff82415158:	66 0f 38 82 44 24 10 	invpcid 0x10(%rsp),%rax

Makes sense?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/cpu] x86/cpu: Use INVPCID mnemonic in invpcid.h

[Uros Bizjak]

On Tue, May 12, 2020 at 5:15 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, May 12, 2020 at 04:26:37PM +0200, Uros Bizjak wrote:
> > Actually, the order was correct for AT&T syntax in the original patch.
> >
> > The insn template for AT&T syntax goes:
> >
> > insn arg2, arg1, arg0
> >
> > where rightmost arguments are output operands.
> >
> > The operands in asm template go
> >
> > asm ("insn template" : output0, output1 : input0, input1 : clobbers)
> >
> > so, in effect:
> >
> > asm ("insn template" : arg0, arg1 : arg2, arg3: clobbers)
> >
> > As you can see, the operand order in insn tempate is reversed for AT&T
> > syntax. I didn't notice the reversal of operands in your improvement.
>
> Your version had:
>
> +       asm volatile ("invpcid %1, %0"
> +                     : : "r" (type), "m" (desc) : "memory");
>
> with "type" being the 0th operand and "desc" being the 1st operand in
> the input operands list.

Correct.

> The order of the operands after the "invpcid" mnemonic are the other way
> around though: you first have %1 which is "desc" and then %0 which is
> the type.

Also correct. Because AT&T has right-to-left operand order, while asm
statement has left-to-right operand order.

> I simply swapped the arguments order in the input operands list, after
> the second ':'
>
> +       asm volatile("invpcid %[desc], %[type]"
> +                    :: [desc] "m" (desc), [type] "r" (type) : "memory");
>
> so that "desc" comes first and "type" second when reading from
> left-to-right in both

But this is not correct, AT&T insn template should have right-to-left order.

> 1. *after* the "invpcid" mnemonic and
> 2. in the input operands list, after the second ':'.

This is not the case throughout the kernel source. Please see for
example sync_bitops, where:

    asm volatile("lock; " __ASM_SIZE(bts) " %1,%0"
             : "+m" (ADDR)
             : "Ir" (nr)
             : "memory");

(to support my claim, I tried to find instruction with two input
operands; there are some in KVM, but these were also written by
myself).

> And since I'm using the symbolic operand names, then the order just
> works because looking at a before-and-after thing doesn't show any
> opcode differences:

Symbolic operands are agnostic to the position in the asm clause, so
it really doesn't matter much. It just doesn't feel right, when other
cases follow different order.

> $ diff -suprN /tmp/before /tmp/after
> Files /tmp/before and /tmp/after are identical

Sure, otherwise assembler would complain.

> Makes sense?

Well, I don't want to bikeshed around this anymore, so any way is good.

Uros.

Re: [tip: x86/cpu] x86/cpu: Use INVPCID mnemonic in invpcid.h

[Borislav Petkov]

On Tue, May 12, 2020 at 05:54:49PM +0200, Uros Bizjak wrote:
> Symbolic operands are agnostic to the position in the asm clause, so
> it really doesn't matter much. It just doesn't feel right, when other
> cases follow different order.
> 
> > $ diff -suprN /tmp/before /tmp/after
> > Files /tmp/before and /tmp/after are identical
> 
> Sure, otherwise assembler would complain.
> 
> > Makes sense?
> 
> Well, I don't want to bikeshed around this anymore, so any way is good.

Look at it this way: the symbolic operand names feature has made inline
assembly *orders* of magnitude more readable than what it was before.
Kernel folks, including myself, have stumbled upon the question which
operand is which, on a regular basis and having the operand names there
makes reading the inline asm almost trivial.

So while we should not convert wholesale, I think we should aim to
gradually convert those other cases to the a-lot-more readable variant
with symbolic operand names.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette