Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1311015 > unrolled thread
| Started by | Brian Gerst <brgerst@gmail.com> |
|---|---|
| First post | 2016-01-16 20:30 +0100 |
| Last post | 2016-01-20 17:20 +0100 |
| Articles | 20 on this page of 42 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] x86: static_cpu_has_safe: discard dynamic check after init Brian Gerst <brgerst@gmail.com> - 2016-01-16 20:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-16 20:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Brian Gerst <brgerst@gmail.com> - 2016-01-16 21:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-17 11:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Brian Gerst <brgerst@gmail.com> - 2016-01-18 18:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Andy Lutomirski <luto@amacapital.net> - 2016-01-18 18:50 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-18 19:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Andy Lutomirski <luto@amacapital.net> - 2016-01-18 19:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-18 19:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-18 20:50 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 00:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-19 00:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 00:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 15:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 17:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 00:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Andy Lutomirski <luto@amacapital.net> - 2016-01-20 00:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Boris Petkov <bp@suse.de> - 2016-01-20 01:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 05:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 11:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 11:50 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-21 23:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-21 23:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-22 00:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-22 00:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-22 00:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-22 11:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-18 20:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 02:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-19 02:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-19 10:30 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 05:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Brian Gerst <brgerst@gmail.com> - 2016-01-20 05:40 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 05:50 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 12:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 12:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 15:50 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 12:00 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 16:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 16:20 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init Borislav Petkov <bp@suse.de> - 2016-01-20 17:10 +0100
Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 17:20 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 11:50 +0100 |
| Message-ID | <qSYuT-5ao-39@gated-at.bofh.it> |
| In reply to | #1313096 |
On January 20, 2016 2:33:45 AM PST, Borislav Petkov <bp@suse.de> wrote:
>On Tue, Jan 19, 2016 at 08:03:55PM -0800, H. Peter Anvin wrote:
>> On 01/19/16 05:57, Borislav Petkov wrote:
>> >
>> > old insn VA: 0xffffffff8108c0d0, CPU feat: X86_BUG_SYSRET_SS_ATTRS,
>size: 2, padlen: 0
>> > __switch_to:
>> > ffffffff8108c0d0: eb 70 jmp
>ffffffff8108c142
>> > repl insn: 0xffffffff8ca7a1a7, size: 0
>> >
>>
>> This is the only one I could possibly imagine mattering. Would it be
>> possible to get the disassembly here?
>
>Sure, I've kept the vmlinux, see below:
>
>ffffffff8108c0d0: eb 70 jmp ffffffff8108c142
><__switch_to+0x838>
>
>jumps to:
>
>ffffffff8108c142: 48 ff 05 b7 08 aa 0b incq 0xbaa08b7(%rip)
>
>which is something-gcov. In any case, it jumps over the SS fixup code:
>
> if (static_cpu_has_bug(X86_BUG_SYSRET_SS_ATTRS)) {
>
> ...
>
> unsigned short ss_sel;
> savesegment(ss, ss_sel);
> if (ss_sel != __KERNEL_DS)
> loadsegment(ss, __KERNEL_DS);
> }
>
>I guess we could fix that with an alternative_call() where @oldinstr is
>empty so that on !X86_BUG_SYSRET_SS_ATTRS machines we simply return and
>on the others, we do a CALL fixup_ss or so.
>
>We have to pay attention to clobbers, though, similar to the POPCNT fun
>in __arch_hweight32().
>
>ffffffff8108b90a <__switch_to>:
>ffffffff8108b90a: 55 push %rbp
>ffffffff8108b90b: 48 8d 87 c0 2d 00 00 lea
>0x2dc0(%rdi),%rax
>ffffffff8108b912: 48 c7 c1 80 f6 1d 00 mov $0x1df680,%rcx
>ffffffff8108b919: 48 89 e5 mov %rsp,%rbp
>ffffffff8108b91c: 41 57 push %r15
>ffffffff8108b91e: 45 31 ff xor %r15d,%r15d
>ffffffff8108b921: 41 56 push %r14
>
>...
>
>ffffffff8108c0c2: 48 ff 05 07 09 aa 0b incq 0xbaa0907(%rip)
> # ffffffff8cb2c9d0 <__gcov0.__switch_to+0xc0>
>ffffffff8108c0c9: 48 ff 05 08 09 aa 0b incq 0xbaa0908(%rip)
> # ffffffff8cb2c9d8 <__gcov0.__switch_to+0xc8>
>ffffffff8108c0d0: eb 70 jmp ffffffff8108c142
><__switch_to+0x838>
>ffffffff8108c0d2: 48 ff 05 07 09 aa 0b incq 0xbaa0907(%rip)
> # ffffffff8cb2c9e0 <__gcov0.__switch_to+0xd0>
>ffffffff8108c0d9: 48 ff 05 08 09 aa 0b incq 0xbaa0908(%rip)
> # ffffffff8cb2c9e8 <__gcov0.__switch_to+0xd8>
>ffffffff8108c0e0: 48 ff 05 11 09 aa 0b incq 0xbaa0911(%rip)
> # ffffffff8cb2c9f8 <__gcov0.__switch_to+0xe8>
>ffffffff8108c0e7: 66 8c d0 mov %ss,%ax
>ffffffff8108c0ea: 66 83 f8 18 cmp $0x18,%ax
>ffffffff8108c0ee: 75 17 jne ffffffff8108c107
><__switch_to+0x7fd>
>ffffffff8108c0f0: eb 57 jmp ffffffff8108c149
><__switch_to+0x83f>
>ffffffff8108c0f2: 48 ff 05 cf 08 aa 0b incq 0xbaa08cf(%rip)
> # ffffffff8cb2c9c8 <__gcov0.__switch_to+0xb8>
>ffffffff8108c0f9: e8 db b0 01 00 callq ffffffff810a71d9
><warn_pre_alternatives>
>ffffffff8108c0fe: 48 ff 05 eb 08 aa 0b incq 0xbaa08eb(%rip)
> # ffffffff8cb2c9f0 <__gcov0.__switch_to+0xe0>
>ffffffff8108c105: eb 3b jmp ffffffff8108c142
><__switch_to+0x838>
>ffffffff8108c107: 48 ff 05 fa 08 aa 0b incq 0xbaa08fa(%rip)
> # ffffffff8cb2ca08 <__gcov0.__switch_to+0xf8>
>ffffffff8108c10e: b8 18 00 00 00 mov $0x18,%eax
>ffffffff8108c113: 8e d0 mov %eax,%ss
>ffffffff8108c115: 48 ff 05 f4 08 aa 0b incq 0xbaa08f4(%rip)
> # ffffffff8cb2ca10 <__gcov0.__switch_to+0x100>
>ffffffff8108c11c: eb 2b jmp ffffffff8108c149
><__switch_to+0x83f>
>ffffffff8108c11e: 48 ff 05 9b 02 aa 0b incq 0xbaa029b(%rip)
> # ffffffff8cb2c3c0 <__gcov0.copy_xregs_to_kernel+0x30>
>ffffffff8108c125: e9 91 f9 ff ff jmpq ffffffff8108babb
><__switch_to+0x1b1>
>ffffffff8108c12a: 48 ff 05 bf 02 aa 0b incq 0xbaa02bf(%rip)
> # ffffffff8cb2c3f0 <__gcov0.copy_kernel_to_xregs+0x20>
>ffffffff8108c131: e9 84 fe ff ff jmpq ffffffff8108bfba
><__switch_to+0x6b0>
>ffffffff8108c136: 48 ff 05 43 02 aa 0b incq 0xbaa0243(%rip)
> # ffffffff8cb2c380 <__gcov0.copy_kernel_to_fxregs+0x20>
>ffffffff8108c13d: e9 db fe ff ff jmpq ffffffff8108c01d
><__switch_to+0x713>
>ffffffff8108c142: 48 ff 05 b7 08 aa 0b incq 0xbaa08b7(%rip)
> # ffffffff8cb2ca00 <__gcov0.__switch_to+0xf0>
>ffffffff8108c149: 48 83 c4 28 add $0x28,%rsp
>ffffffff8108c14d: 4c 89 e0 mov %r12,%rax
>ffffffff8108c150: 5b pop %rbx
>ffffffff8108c151: 41 5c pop %r12
>ffffffff8108c153: 41 5d pop %r13
>ffffffff8108c155: 41 5e pop %r14
>ffffffff8108c157: 41 5f pop %r15
>ffffffff8108c159: 5d pop %rbp
>ffffffff8108c15a: c3 retq
Ah. What would be even more of a win would be to rebias static_cpu_has_bug() so that the fallthrough case is the functional one. Easily done by reversing the labels.
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-21 23:20 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qTvKa-37P-15@gated-at.bofh.it> |
| In reply to | #1313108 |
On Wed, Jan 20, 2016 at 02:41:22AM -0800, H. Peter Anvin wrote:
> Ah. What would be even more of a win would be to rebias
> static_cpu_has_bug() so that the fallthrough case is the functional
> one. Easily done by reversing the labels.
By reversing you mean this:
---
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 77c51f4c15b7..49fa56f2b083 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -174,10 +174,10 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
[bitnum] "i" (1 << (bit & 7)),
[cap_word] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
: : t_yes, t_no);
- t_yes:
- return true;
t_no:
return false;
+ t_yes:
+ return true;
#else
return boot_cpu_has(bit);
#endif /* CC_HAVE_ASM_GOTO */
---
?
In any case, here's what happens with the current patchset:
vmlinux:
ffffffff8100472a: e9 50 0e de 00 jmpq ffffffff81de557f <__alt_instructions_end+0x7aa>
ffffffff8100472f: 66 8c d0 mov %ss,%ax
ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax
ffffffff81004736: 74 07 je ffffffff8100473f <__switch_to+0x2ef>
ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax
ffffffff8100473d: 8e d0 mov %eax,%ss
ffffffff8100473f: 48 83 c4 18 add $0x18,%rsp
ffffffff81004743: 4c 89 e0 mov %r12,%rax
ffffffff81004746: 5b pop %rbx
ffffffff81004747: 41 5c pop %r12
ffffffff81004749: 41 5d pop %r13
ffffffff8100474b: 41 5e pop %r14
ffffffff8100474d: 41 5f pop %r15
ffffffff8100474f: 5d pop %rbp
ffffffff81004750: c3 retq
That first JMP above sends us to the dynamic section which is in asm now:
ffffffff81de557f: f6 05 8f de d1 ff 01 testb $0x1,-0x2e2171(%rip) # ffffffff81b03415 <boot_cpu_data+0x55>
ffffffff81de5586: 0f 85 a3 f1 21 ff jne ffffffff8100472f <__switch_to+0x2df>
ffffffff81de558c: e9 ae f1 21 ff jmpq ffffffff8100473f <__switch_to+0x2ef>
After X86_FEATURE_ALWAYS patching, that first JMP has become a 2-byte JMP:
[ 0.306333] apply_alternatives: feat: 3*32+21, old: (ffffffff8100472a, len: 5), repl: (ffffffff81de4e12, len: 5), pad: 0
[ 0.308005] ffffffff8100472a: old_insn: e9 50 0e de 00
[ 0.312012] ffffffff81de4e12: rpl_insn: e9 28 f9 21 ff
[ 0.318201] recompute_jump: target RIP: ffffffff8100473f, new_displ: 0x15
[ 0.320007] recompute_jump: final displ: 0x00000013, JMP 0xffffffff8100473f
[ 0.324005] ffffffff8100472a: final_insn: eb 13 0f 1f 00
so basically we jump over the %ss fixup:
ffffffff8100472a: eb 13 0f 1f 00 jmp ffffffff8100473f
ffffffff8100472f: 66 8c d0 mov %ss,%ax
ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax
ffffffff81004736: 74 07 je ffffffff8100473f <__switch_to+0x2ef>
ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax
ffffffff8100473d: 8e d0 mov %eax,%ss
ffffffff8100473f: 48 83 c4 18 add $0x18,%rsp <----
ffffffff81004743: 4c 89 e0 mov %r12,%rax
ffffffff81004746: 5b pop %rbx
ffffffff81004747: 41 5c pop %r12
ffffffff81004749: 41 5d pop %r13
ffffffff8100474b: 41 5e pop %r14
ffffffff8100474d: 41 5f pop %r15
ffffffff8100474f: 5d pop %rbp
ffffffff81004750: c3 retq
After X86_BUG_SYSRET_SS_ATTRS patching:
[ 0.330367] apply_alternatives: feat: 16*32+8, old: (ffffffff8100472a, len: 5), repl: (ffffffff81de3996, len: 0), pad: 0
[ 0.332005] ffffffff8100472a: old_insn: eb 13 0f 1f 00
[ 0.338332] ffffffff8100472a: final_insn: 0f 1f 44 00 00
ffffffff8100472a: 0f 1f 44 00 00 nop
ffffffff8100472f: 66 8c d0 mov %ss,%ax
ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax
ffffffff81004736: 74 07 je ffffffff8100473f <__switch_to+0x2ef>
ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax
ffffffff8100473d: 8e d0 mov %eax,%ss
ffffffff8100473f: 48 83 c4 18 add $0x18,%rsp
ffffffff81004743: 4c 89 e0 mov %r12,%rax
ffffffff81004746: 5b pop %rbx
ffffffff81004747: 41 5c pop %r12
ffffffff81004749: 41 5d pop %r13
ffffffff8100474b: 41 5e pop %r14
ffffffff8100474d: 41 5f pop %r15
ffffffff8100474f: 5d pop %rbp
ffffffff81004750: c3 retq
So the penalty for the !X86_BUG_SYSRET_SS_ATTRS CPUs is a 2-byte JMP. Do
we care?
In the case we do, we could do this:
JMP ss_fixup
ret:
RET return prev_p;
ss_fixup:
<fixup SS>
jmp ret
and the !X86_BUG_SYSRET_SS_ATTRS CPUs would overwrite that
"JMP ss_fixup" with a NOP and they're fine. However, the
X86_BUG_SYSRET_SS_ATTRS CPUs will have to do two jumps, one to the fixup
code and one back to RET.
Now, how about I convert
unsigned short ss_sel;
savesegment(ss, ss_sel);
if (ss_sel != __KERNEL_DS)
loadsegment(ss, __KERNEL_DS);
into asm and into an alternative()?
Then, the !X86_BUG_SYSRET_SS_ATTRS CPUs will trade off that JMP with a
bunch of NOPs which will pollute I$.
Hmmm.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-21 23:30 +0100 |
| Message-ID | <qTvTR-3bF-51@gated-at.bofh.it> |
| In reply to | #1314567 |
On January 21, 2016 2:14:42 PM PST, Borislav Petkov <bp@suse.de> wrote: >On Wed, Jan 20, 2016 at 02:41:22AM -0800, H. Peter Anvin wrote: >> Ah. What would be even more of a win would be to rebias >> static_cpu_has_bug() so that the fallthrough case is the functional >> one. Easily done by reversing the labels. > >By reversing you mean this: > > >--- >diff --git a/arch/x86/include/asm/cpufeature.h >b/arch/x86/include/asm/cpufeature.h >index 77c51f4c15b7..49fa56f2b083 100644 >--- a/arch/x86/include/asm/cpufeature.h >+++ b/arch/x86/include/asm/cpufeature.h >@@ -174,10 +174,10 @@ static __always_inline __pure bool >_static_cpu_has(u16 bit) > [bitnum] "i" (1 << (bit & 7)), >[cap_word] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3]) > : : t_yes, t_no); >- t_yes: >- return true; > t_no: > return false; >+ t_yes: >+ return true; > #else > return boot_cpu_has(bit); > #endif /* CC_HAVE_ASM_GOTO */ >--- > >? > >In any case, here's what happens with the current patchset: > >vmlinux: > >ffffffff8100472a: e9 50 0e de 00 jmpq ffffffff81de557f ><__alt_instructions_end+0x7aa> >ffffffff8100472f: 66 8c d0 mov %ss,%ax >ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax >ffffffff81004736: 74 07 je ffffffff8100473f ><__switch_to+0x2ef> >ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax >ffffffff8100473d: 8e d0 mov %eax,%ss >ffffffff8100473f: 48 83 c4 18 add $0x18,%rsp >ffffffff81004743: 4c 89 e0 mov %r12,%rax >ffffffff81004746: 5b pop %rbx >ffffffff81004747: 41 5c pop %r12 >ffffffff81004749: 41 5d pop %r13 >ffffffff8100474b: 41 5e pop %r14 >ffffffff8100474d: 41 5f pop %r15 >ffffffff8100474f: 5d pop %rbp >ffffffff81004750: c3 retq > >That first JMP above sends us to the dynamic section which is in asm >now: > >ffffffff81de557f: f6 05 8f de d1 ff 01 testb >$0x1,-0x2e2171(%rip) # ffffffff81b03415 <boot_cpu_data+0x55> >ffffffff81de5586: 0f 85 a3 f1 21 ff jne ffffffff8100472f ><__switch_to+0x2df> >ffffffff81de558c: e9 ae f1 21 ff jmpq ffffffff8100473f ><__switch_to+0x2ef> > >After X86_FEATURE_ALWAYS patching, that first JMP has become a 2-byte >JMP: > >[ 0.306333] apply_alternatives: feat: 3*32+21, old: >(ffffffff8100472a, len: 5), repl: (ffffffff81de4e12, len: 5), pad: 0 >[ 0.308005] ffffffff8100472a: old_insn: e9 50 0e de 00 >[ 0.312012] ffffffff81de4e12: rpl_insn: e9 28 f9 21 ff >[ 0.318201] recompute_jump: target RIP: ffffffff8100473f, new_displ: >0x15 >[ 0.320007] recompute_jump: final displ: 0x00000013, JMP >0xffffffff8100473f >[ 0.324005] ffffffff8100472a: final_insn: eb 13 0f 1f 00 > >so basically we jump over the %ss fixup: > >ffffffff8100472a: eb 13 0f 1f 00 jmp ffffffff8100473f >ffffffff8100472f: 66 8c d0 mov %ss,%ax >ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax >ffffffff81004736: 74 07 je ffffffff8100473f ><__switch_to+0x2ef> >ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax >ffffffff8100473d: 8e d0 mov %eax,%ss >ffffffff8100473f: 48 83 c4 18 add >$0x18,%rsp <---- >ffffffff81004743: 4c 89 e0 mov %r12,%rax >ffffffff81004746: 5b pop %rbx >ffffffff81004747: 41 5c pop %r12 >ffffffff81004749: 41 5d pop %r13 >ffffffff8100474b: 41 5e pop %r14 >ffffffff8100474d: 41 5f pop %r15 >ffffffff8100474f: 5d pop %rbp >ffffffff81004750: c3 retq > > >After X86_BUG_SYSRET_SS_ATTRS patching: > >[ 0.330367] apply_alternatives: feat: 16*32+8, old: >(ffffffff8100472a, len: 5), repl: (ffffffff81de3996, len: 0), pad: 0 >[ 0.332005] ffffffff8100472a: old_insn: eb 13 0f 1f 00 >[ 0.338332] ffffffff8100472a: final_insn: 0f 1f 44 00 00 > >ffffffff8100472a: 0f 1f 44 00 00 nop >ffffffff8100472f: 66 8c d0 mov %ss,%ax >ffffffff81004732: 66 83 f8 18 cmp $0x18,%ax >ffffffff81004736: 74 07 je ffffffff8100473f ><__switch_to+0x2ef> >ffffffff81004738: b8 18 00 00 00 mov $0x18,%eax >ffffffff8100473d: 8e d0 mov %eax,%ss >ffffffff8100473f: 48 83 c4 18 add $0x18,%rsp >ffffffff81004743: 4c 89 e0 mov %r12,%rax >ffffffff81004746: 5b pop %rbx >ffffffff81004747: 41 5c pop %r12 >ffffffff81004749: 41 5d pop %r13 >ffffffff8100474b: 41 5e pop %r14 >ffffffff8100474d: 41 5f pop %r15 >ffffffff8100474f: 5d pop %rbp >ffffffff81004750: c3 retq > >So the penalty for the !X86_BUG_SYSRET_SS_ATTRS CPUs is a 2-byte JMP. >Do >we care? > >In the case we do, we could do this: > > JMP ss_fixup >ret: > RET return prev_p; >ss_fixup: > <fixup SS> > jmp ret > >and the !X86_BUG_SYSRET_SS_ATTRS CPUs would overwrite that >"JMP ss_fixup" with a NOP and they're fine. However, the >X86_BUG_SYSRET_SS_ATTRS CPUs will have to do two jumps, one to the >fixup >code and one back to RET. > >Now, how about I convert > > unsigned short ss_sel; > savesegment(ss, ss_sel); > if (ss_sel != __KERNEL_DS) > loadsegment(ss, __KERNEL_DS); > >into asm and into an alternative()? > >Then, the !X86_BUG_SYSRET_SS_ATTRS CPUs will trade off that JMP with a >bunch of NOPs which will pollute I$. > >Hmmm. Yes, having t_no as the fallthrough case ought to move the yes code out of line. The current code probably pollutes the i$ too. -- Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-22 00:00 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qTwmV-3nT-75@gated-at.bofh.it> |
| In reply to | #1314577 |
On Thu, Jan 21, 2016 at 02:22:28PM -0800, H. Peter Anvin wrote:
> Yes, having t_no as the fallthrough case ought to move the yes code
> out of line.
Dunno, maybe I'm doing something wrong:
I have this change:
---
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 7f09de998c93..f9833fcb8fcb 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -175,10 +175,10 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
[bitnum] "i" (1 << (bit & 7)),
[cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
: : t_yes, t_no);
- t_yes:
- return true;
t_no:
return false;
+ t_yes:
+ return true;
}
#define static_cpu_has(bit) \
---
and the resulting code looks even wrong (or my brain is fried for today
- one of the two).
vmlinux:
ffffffff810046ae: e9 cc 0e de 00 jmpq ffffffff81de557f <__alt_instructions_end+0x7aa>
ffffffff810046b3: 48 83 c4 18 add $0x18,%rsp
ffffffff810046b7: 4c 89 e0 mov %r12,%rax
ffffffff810046ba: 5b pop %rbx
ffffffff810046bb: 41 5c pop %r12
ffffffff810046bd: 41 5d pop %r13
ffffffff810046bf: 41 5e pop %r14
ffffffff810046c1: 41 5f pop %r15
ffffffff810046c3: 5d pop %rbp
ffffffff810046c4: c3 retq
dynamic branch:
ffffffff81de557f: f6 05 8f de d1 ff 01 testb $0x1,-0x2e2171(%rip) # ffffffff81b03415 <boot_cpu_data+0x55>
ffffffff81de5586: 0f 85 f6 f1 21 ff jne ffffffff81004782 <__switch_to+0x332>
ffffffff81de558c: e9 22 f1 21 ff jmpq ffffffff810046b3 <__switch_to+0x263>
after X86_FEATURE_ALWAYS patching:
[ 0.288007] apply_alternatives: feat: 3*32+21, old: (ffffffff810046ae, len: 5), repl: (ffffffff81de4dff, len: 5), pad: 0
[ 0.292004] ffffffff810046ae: old_insn: e9 cc 0e de 00
[ 0.300013] ffffffff81de4dff: rpl_insn: e9 af f8 21 ff
[ 0.308006] recompute_jump: target RIP: ffffffff810046b3, new_displ: 0x5
[ 0.312006] recompute_jump: final displ: 0x00000003, JMP 0xffffffff810046b3
[ 0.316006] ffffffff810046ae: final_insn: eb 03 0f 1f 00
ffffffff810046ae: eb 03 0f 1f 00 jmp ffffffff810046b3 ---
ffffffff810046b3: 48 83 c4 18 add $0x18,%rsp <--
ffffffff810046b7: 4c 89 e0 mov %r12,%rax
ffffffff810046ba: 5b pop %rbx
ffffffff810046bb: 41 5c pop %r12
ffffffff810046bd: 41 5d pop %r13
ffffffff810046bf: 41 5e pop %r14
ffffffff810046c1: 41 5f pop %r15
ffffffff810046c3: 5d pop %rbp
ffffffff810046c4: c3 retq
so this is silly: we're basically jumping after the JMP instruction
itself. So that will be the case on !X86_BUG_SYSRET_SS_ATTRS CPUs.
Still a two-byte and now even a useless JMP.
The right thing to do would be to generate a NOP simply.
On X86_BUG_SYSRET_SS_ATTRS CPUs:
[ 0.322014] apply_alternatives: feat: 16*32+8, old: (ffffffff810046ae, len: 5), repl: (ffffffff81de3962, len: 0), pad: 0
[ 0.324005] ffffffff810046ae: old_insn: eb 03 0f 1f 00
[ 0.332006] ffffffff810046ae: final_insn: 0f 1f 44 00 00
ffffffff810046ae: 0f 1f 44 00 00 nop
ffffffff810046b3: 48 83 c4 18 add $0x18,%rsp
ffffffff810046b7: 4c 89 e0 mov %r12,%rax
ffffffff810046ba: 5b pop %rbx
ffffffff810046bb: 41 5c pop %r12
ffffffff810046bd: 41 5d pop %r13
ffffffff810046bf: 41 5e pop %r14
ffffffff810046c1: 41 5f pop %r15
ffffffff810046c3: 5d pop %rbp
ffffffff810046c4: c3 retq
which is actually even wrong!
What it should've done is
jne ffffffff81004782
as the dynamic code did. At that address we have the ss fixup:
ffffffff81004782: 66 8c d0 mov %ss,%ax
ffffffff81004785: 66 83 f8 18 cmp $0x18,%ax
ffffffff81004789: 0f 84 24 ff ff ff je ffffffff810046b3 <__switch_to+0x263>
ffffffff8100478f: b8 18 00 00 00 mov $0x18,%eax
ffffffff81004794: 8e d0 mov %eax,%ss
ffffffff81004796: e9 18 ff ff ff jmpq ffffffff810046b3 <__switch_to+0x263>
with the jump back to the ret code. Which means,
!X86_BUG_SYSRET_SS_ATTRS CPUs get to do a forward and a backward JMP. So
even if it did the right thing, it would be two JMPs.
Meh.
I need to think about something better.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-22 00:40 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qTwZA-3SW-9@gated-at.bofh.it> |
| In reply to | #1314614 |
On 01/21/16 14:56, Borislav Petkov wrote: > > so this is silly: we're basically jumping after the JMP instruction > itself. So that will be the case on !X86_BUG_SYSRET_SS_ATTRS CPUs. > Still a two-byte and now even a useless JMP. > > The right thing to do would be to generate a NOP simply. > OK, so gcc isn't as clever as I thought. -hpa
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-22 00:40 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qTwZA-3SW-17@gated-at.bofh.it> |
| In reply to | #1314614 |
Maybe a label attribute would help, I don't know. -hpa
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-22 11:40 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qTHii-2Nr-9@gated-at.bofh.it> |
| In reply to | #1314644 |
On Thu, Jan 21, 2016 at 03:37:16PM -0800, H. Peter Anvin wrote:
> Maybe a label attribute would help, I don't know.
Here's another version which works, not really better though:
Change is this:
---
+ asm_volatile_goto(ALTERNATIVE("", "jmp %l[t_fixup_ss]",
+ X86_BUG_SYSRET_SS_ATTRS)
+ : : : : t_fixup_ss);
+
+ return prev_p;
+
+t_fixup_ss:
<snip comment>
+ savesegment(ss, ss_sel);
+ if (ss_sel != __KERNEL_DS)
+ loadsegment(ss, __KERNEL_DS);
return prev_p;
---
with two "return prev_p" with the hope that gcc won't generate a second
JMP back to the frame restore and ret code. But, nah, it does.
vmlinux:
ffffffff8100472a: 90 nop
ffffffff8100472b: 90 nop
ffffffff8100472c: 90 nop
ffffffff8100472d: 90 nop
ffffffff8100472e: 90 nop
ffffffff8100472f: 48 83 c4 18 add $0x18,%rsp
ffffffff81004733: 4c 89 e0 mov %r12,%rax
ffffffff81004736: 5b pop %rbx
ffffffff81004737: 41 5c pop %r12
ffffffff81004739: 41 5d pop %r13
ffffffff8100473b: 41 5e pop %r14
ffffffff8100473d: 41 5f pop %r15
ffffffff8100473f: 5d pop %rbp
ffffffff81004740: c3 retq
after patching on an X86_BUG_SYSRET_SS_ATTRS CPU:
[ 0.264007] apply_alternatives: feat: 16*32+8, old: (ffffffff8100472a, len: 5), repl: (ffffffff81de4e05, len: 5), pad: 5
[ 0.268005] ffffffff8100472a: old_insn: 90 90 90 90 90
[ 0.273510] ffffffff81de4e05: rpl_insn: e9 68 f9 21 ff
[ 0.277496] recompute_jump: target RIP: ffffffff81004772, new_displ: 0x48
[ 0.280005] recompute_jump: final displ: 0x00000046, JMP 0xffffffff81004772
[ 0.283159] ffffffff8100472a: final_insn: eb 46 0f 1f 00
ffffffff8100472a: eb 46 0f 1f 00 jmp ffffffff81004772
ffffffff8100472f: 48 83 c4 18 add $0x18,%rsp
ffffffff81004733: 4c 89 e0 mov %r12,%rax
ffffffff81004736: 5b pop %rbx
ffffffff81004737: 41 5c pop %r12
ffffffff81004739: 41 5d pop %r13
ffffffff8100473b: 41 5e pop %r14
ffffffff8100473d: 41 5f pop %r15
ffffffff8100473f: 5d pop %rbp
ffffffff81004740: c3 retq
so a two-byte JMP jumping to:
ffffffff81004772: 66 8c d0 mov %ss,%ax
ffffffff81004775: 66 83 f8 18 cmp $0x18,%ax
ffffffff81004779: 74 b4 je ffffffff8100472f <__switch_to+0x2df>
ffffffff8100477b: b8 18 00 00 00 mov $0x18,%eax
ffffffff81004780: 8e d0 mov %eax,%ss
ffffffff81004782: eb ab jmp ffffffff8100472f <__switch_to+0x2df>
which does the fixup and jumps back to ...472f which restores the frame
and returns.
I wish I could be able to tell gcc to not jump back but add the function
return here too as we don't care about code size in that case.
And it's not like it is really better on !X86_BUG_SYSRET_SS_ATTRS CPUs -
there we have the 5-byte padding NOP being converted to 5-byte one:
[ 0.293164] ffffffff8100472a: [0:5) optimized NOPs: 0f 1f 44 00 00
I need to talk to my gcc guy... :)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-18 20:00 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSnbY-4FZ-15@gated-at.bofh.it> |
| In reply to | #1311734 |
On Mon, Jan 18, 2016 at 07:14:57PM +0100, Borislav Petkov wrote:
> So we're talking about less than a page here?
>
> [19] .static_cpu_has PROGBITS ffffffff81d335d3 011335d3
> 00000000000002df 0000000000000000 AX 0 0 1
>
> That's 479 bytes. Meh, it doesn't look like it is worth the trouble.
>
> Maybe I should build an allyesconfig.
[45] .static_cpu_has PROGBITS ffffffff97aa655b 16ea655b
00000000000002df 0000000000000000 AX 0 0 1
Same.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-19 02:20 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSt7I-vW-11@gated-at.bofh.it> |
| In reply to | #1311746 |
On Mon, Jan 18, 2016 at 07:51:07PM +0100, Borislav Petkov wrote:
> [45] .static_cpu_has PROGBITS ffffffff97aa655b 16ea655b
> 00000000000002df 0000000000000000 AX 0 0 1
Ok, staring at this section was wrong. I went and looked at
the .s file and now it clicked: gcc inlines those calls to
__static_cpu_has_safe of the dynamic jump target:
.loc 4 538 0
movl $125, %edi #,
call __static_cpu_has_safe #
.LBE885:
.LBE886:
.LBE914:
.loc 1 240 0
testb %al, %al # D.30157
je .L150 #,
jmp .L151 #
.L152:
.LBB915:
.LBB909:
.LBB905:
.LBB893:
.LBB892:
.loc 4 538 0
movl $154, %edi #,
call __static_cpu_has_safe #
.LBE892:
.LBE893:
.LBE905:
.loc 7 431 0
testb %al, %al # D.30157
jne .L154 #,
which turn into:
751: bf 7d 00 00 00 mov $0x7d,%edi
756: e8 00 00 00 00 callq 75b <fpu__copy+0xab>
75b: 84 c0 test %al,%al
75d: 74 a3 je 702 <fpu__copy+0x52>
75f: eb 90 jmp 6f1 <fpu__copy+0x41>
761: bf 9a 00 00 00 mov $0x9a,%edi
766: e8 00 00 00 00 callq 76b <fpu__copy+0xbb>
76b: 84 c0 test %al,%al
That's like 28 bytes in this particular case, which is 14 per call site,
on average.
So grepping through my tailored vmlinux, it has 35 entries with
X86_FEATURE_ALWAYS in .altinstructions X 14 = 490 bytes.
So yeah, we probably should do this, allyesconfig should give more
savings.
One thing I'd probably do differently is not call the throwaway section
.static_cpu_has but something like .altinstr_temporary or so and put it
after the replacement insns:
.altinstr_replacement : AT(ADDR(.altinstr_replacement) - LOAD_OFFSET) {
*(.altinstr_replacement)
*(.altinstr_temporary)
}
so that we know those instructions belong to the alternatives mechanism.
They'll get discared too, of course.
I could just as well be talking a lot of crap, it is waay too late here.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-19 02:40 +0100 |
| Message-ID | <qStr4-D8-1@gated-at.bofh.it> |
| In reply to | #1311880 |
On January 18, 2016 5:10:26 PM PST, Borislav Petkov <bp@suse.de> wrote:
>On Mon, Jan 18, 2016 at 07:51:07PM +0100, Borislav Petkov wrote:
>> [45] .static_cpu_has PROGBITS ffffffff97aa655b 16ea655b
>> 00000000000002df 0000000000000000 AX 0 0 1
>
>Ok, staring at this section was wrong. I went and looked at
>the .s file and now it clicked: gcc inlines those calls to
>__static_cpu_has_safe of the dynamic jump target:
>
> .loc 4 538 0
> movl $125, %edi #,
> call __static_cpu_has_safe #
>.LBE885:
>.LBE886:
>.LBE914:
> .loc 1 240 0
> testb %al, %al # D.30157
> je .L150 #,
> jmp .L151 #
>.L152:
>.LBB915:
>.LBB909:
>.LBB905:
>.LBB893:
>.LBB892:
> .loc 4 538 0
> movl $154, %edi #,
> call __static_cpu_has_safe #
>.LBE892:
>.LBE893:
>.LBE905:
> .loc 7 431 0
> testb %al, %al # D.30157
> jne .L154 #,
>
>which turn into:
>
> 751: bf 7d 00 00 00 mov $0x7d,%edi
> 756: e8 00 00 00 00 callq 75b <fpu__copy+0xab>
> 75b: 84 c0 test %al,%al
> 75d: 74 a3 je 702 <fpu__copy+0x52>
> 75f: eb 90 jmp 6f1 <fpu__copy+0x41>
> 761: bf 9a 00 00 00 mov $0x9a,%edi
> 766: e8 00 00 00 00 callq 76b <fpu__copy+0xbb>
> 76b: 84 c0 test %al,%al
>
>That's like 28 bytes in this particular case, which is 14 per call
>site,
>on average.
>
>So grepping through my tailored vmlinux, it has 35 entries with
>X86_FEATURE_ALWAYS in .altinstructions X 14 = 490 bytes.
>
>So yeah, we probably should do this, allyesconfig should give more
>savings.
>
>One thing I'd probably do differently is not call the throwaway section
>.static_cpu_has but something like .altinstr_temporary or so and put it
>after the replacement insns:
>
>.altinstr_replacement : AT(ADDR(.altinstr_replacement) - LOAD_OFFSET) {
> *(.altinstr_replacement)
> *(.altinstr_temporary)
> }
>
>so that we know those instructions belong to the alternatives
>mechanism.
>They'll get discared too, of course.
>
>I could just as well be talking a lot of crap, it is waay too late
>here.
Why the f do we call a subroutine for what amounts to a single bt or test instruction?
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-19 10:30 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSALU-5Mg-9@gated-at.bofh.it> |
| In reply to | #1311883 |
On Mon, Jan 18, 2016 at 05:33:03PM -0800, H. Peter Anvin wrote:
> Why the f do we call a subroutine for what amounts to a single bt or
> test instruction?
No real reason. You can kick me when you see me next time:
4a90a99c4f80 ("x86: Add a static_cpu_has_safe variant")
:-)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 05:10 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSSfM-16M-41@gated-at.bofh.it> |
| In reply to | #1312038 |
On 01/19/16 01:22, Borislav Petkov wrote:
> On Mon, Jan 18, 2016 at 05:33:03PM -0800, H. Peter Anvin wrote:
>> Why the f do we call a subroutine for what amounts to a single bt or
>> test instruction?
>
> No real reason. You can kick me when you see me next time:
>
> 4a90a99c4f80 ("x86: Add a static_cpu_has_safe variant")
>
So, here is my suggestion:
1. Just get rid of static_cpu_has_safe() and make static_cpu_has() safe.
2. Get rid of the non-asm goto variant and just fall back to dynamic if
asm goto is unavailable. It doesn't make any sense, really, if it is
supposed to be safe, and by now the asm goto-capable gcc is in more wide
use. (Originally the gcc 3.x fallback to pure dynamic didn't exist,
either.)
3. Put the dynamic test in the .init.text section and inline it:
.section .init.text,"ax"
testb %2,%3
jnz %[t_yes]
jmp %[t_no]
.previous
... "i" (1 << (bit & 7)),
"m" (((const char *)boot_cpu_data->x86_capability)[bit >> 3]) ...
(The code would be slightly simpler/cleaner with testl, but that would
unnecessarily create a long immediate, or with btl, but that would be
slower. We could use CONST_MASK_ADDR() and CONST_MASK() from
asm/bitops.h, but I'm slightly uncomfortable with the idea of leveraging
an interface which is ultimately an internal implementation detail of
bitops.h that might change in the future without people realizing its
implications.)
The only thing we lose is the 2-byte optimization in case we have the
good luck for it to actually work. At this point I'm thinking it isn't
worth it, and instead that safety trumps it. It *would* be interesting
to see if there are any call sites where it would actually kick in.
-hpa
[toc] | [prev] | [next] | [standalone]
| From | Brian Gerst <brgerst@gmail.com> |
|---|---|
| Date | 2016-01-20 05:40 +0100 |
| Message-ID | <qSSIO-1jE-65@gated-at.bofh.it> |
| In reply to | #1312860 |
On Tue, Jan 19, 2016 at 11:02 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 01/19/16 01:22, Borislav Petkov wrote:
>> On Mon, Jan 18, 2016 at 05:33:03PM -0800, H. Peter Anvin wrote:
>>> Why the f do we call a subroutine for what amounts to a single bt or
>>> test instruction?
>>
>> No real reason. You can kick me when you see me next time:
>>
>> 4a90a99c4f80 ("x86: Add a static_cpu_has_safe variant")
>>
>
> So, here is my suggestion:
>
> 1. Just get rid of static_cpu_has_safe() and make static_cpu_has() safe.
>
> 2. Get rid of the non-asm goto variant and just fall back to dynamic if
> asm goto is unavailable. It doesn't make any sense, really, if it is
> supposed to be safe, and by now the asm goto-capable gcc is in more wide
> use. (Originally the gcc 3.x fallback to pure dynamic didn't exist,
> either.)
>
> 3. Put the dynamic test in the .init.text section and inline it:
>
> .section .init.text,"ax"
> testb %2,%3
> jnz %[t_yes]
> jmp %[t_no]
> .previous
>
> ... "i" (1 << (bit & 7)),
> "m" (((const char *)boot_cpu_data->x86_capability)[bit >> 3]) ...
Can't put it in .init.text or else you get:
WARNING: arch/x86/kernel/built-in.o(.text+0x4b9): Section mismatch in
reference from the function __switch_to() to the (unknown reference)
.init.text:(unknown)
The function __switch_to() references
the (unknown reference) __init (unknown).
This is often because __switch_to lacks a __init
annotation or the annotation of (unknown) is wrong.
We want to override that because we know that the reference will be
removed after alternatives run. That's why I created a new section.
--
Brian Gerst
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 05:50 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSSSu-1pe-35@gated-at.bofh.it> |
| In reply to | #1312869 |
On 01/19/16 20:39, Brian Gerst wrote: > > Can't put it in .init.text or else you get: > WARNING: arch/x86/kernel/built-in.o(.text+0x4b9): Section mismatch in > reference from the function __switch_to() to the (unknown reference) > .init.text:(unknown) > The function __switch_to() references > the (unknown reference) __init (unknown). > This is often because __switch_to lacks a __init > annotation or the annotation of (unknown) is wrong. > > We want to override that because we know that the reference will be > removed after alternatives run. That's why I created a new section. > Right. I wish we could tag reference call sites as clean, not sources or targets. Sigh. However, that's not too much of an issue. -hpa
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 12:00 +0100 |
| Message-ID | <qSYEz-5ea-13@gated-at.bofh.it> |
| In reply to | #1312870 |
On January 20, 2016 2:50:18 AM PST, Borislav Petkov <bp@suse.de> wrote:
>On Tue, Jan 19, 2016 at 08:42:10PM -0800, H. Peter Anvin wrote:
>> Right. I wish we could tag reference call sites as clean, not
>sources
>> or targets. Sigh.
>
>I was thinking about something like this:
>
>From: Borislav Petkov <bp@suse.de>
>Date: Wed, 20 Jan 2016 11:45:36 +0100
>Subject: [PATCH] x86/alternatives: Add an auxilary section
>
>Add .altinstr_aux for additional instructions which will be used before
>and/or during patching. All stuff which needs more sophisticated
>patching should go there. See next patch.
>
>Signed-off-by: Borislav Petkov <bp@suse.de>
>---
> arch/x86/kernel/vmlinux.lds.S | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/arch/x86/kernel/vmlinux.lds.S
>b/arch/x86/kernel/vmlinux.lds.S
>index 74e4bf11f562..35868bf529b9 100644
>--- a/arch/x86/kernel/vmlinux.lds.S
>+++ b/arch/x86/kernel/vmlinux.lds.S
>@@ -244,6 +244,11 @@ SECTIONS
> */
> .altinstr_replacement : AT(ADDR(.altinstr_replacement) - LOAD_OFFSET)
>{
> *(.altinstr_replacement)
>+ /*
>+ * Auxiliary section for misc instruction patching tasks. See
>+ * static_cpu_has(), for an example.
>+ */
>+ *(.altinstr_aux)
> }
>
> /*
How about:
section for code used exclusively before alternatives are run. All references to such code must be patched out by alternatives, normally by using a patch with X86_FEATURE_ALWAYS.
See static_cpu_has() for an example.
--
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-20 12:10 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSYOd-5wS-5@gated-at.bofh.it> |
| In reply to | #1313116 |
On Wed, Jan 20, 2016 at 02:55:24AM -0800, H. Peter Anvin wrote:
> How about:
>
> section for code used exclusively before alternatives are run. All references to such code must be patched out by alternatives, normally by using a patch with X86_FEATURE_ALWAYS.
>
> See static_cpu_has() for an example.
Sure.
My thinking was to make it a bit more generic so that if we decide to do
some different monkey business with the alternatives, to put stuff in
there too.
But we can always change that later - it's not like it is user-visible.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 15:50 +0100 |
| Message-ID | <qT2f8-7Ty-29@gated-at.bofh.it> |
| In reply to | #1313121 |
On January 20, 2016 3:05:19 AM PST, Borislav Petkov <bp@suse.de> wrote: >On Wed, Jan 20, 2016 at 02:55:24AM -0800, H. Peter Anvin wrote: >> How about: >> >> section for code used exclusively before alternatives are run. All >references to such code must be patched out by alternatives, normally >by using a patch with X86_FEATURE_ALWAYS. >> >> See static_cpu_has() for an example. > >Sure. > >My thinking was to make it a bit more generic so that if we decide to >do >some different monkey business with the alternatives, to put stuff in >there too. > >But we can always change that later - it's not like it is user-visible. I don't think the verbiage I suggested in any way disagrees with that notion. -- Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-20 12:00 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qSYEz-5ea-15@gated-at.bofh.it> |
| In reply to | #1312870 |
On Tue, Jan 19, 2016 at 08:42:10PM -0800, H. Peter Anvin wrote:
> Right. I wish we could tag reference call sites as clean, not sources
> or targets. Sigh.
I was thinking about something like this:
From: Borislav Petkov <bp@suse.de>
Date: Wed, 20 Jan 2016 11:45:36 +0100
Subject: [PATCH] x86/alternatives: Add an auxilary section
Add .altinstr_aux for additional instructions which will be used before
and/or during patching. All stuff which needs more sophisticated
patching should go there. See next patch.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/vmlinux.lds.S | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 74e4bf11f562..35868bf529b9 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -244,6 +244,11 @@ SECTIONS
*/
.altinstr_replacement : AT(ADDR(.altinstr_replacement) - LOAD_OFFSET) {
*(.altinstr_replacement)
+ /*
+ * Auxiliary section for misc instruction patching tasks. See
+ * static_cpu_has(), for an example.
+ */
+ *(.altinstr_aux)
}
/*
--
2.3.5
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-01-20 16:10 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qT2yu-8hL-27@gated-at.bofh.it> |
| In reply to | #1312860 |
On Tue, Jan 19, 2016 at 08:02:01PM -0800, H. Peter Anvin wrote:
> ... "i" (1 << (bit & 7)),
> "m" (((const char *)boot_cpu_data->x86_capability)[bit >> 3]) ...
Nice!
I was going to do:
"i" (1 << (bit & 31)),
"m" (((boot_cpu_data->x86_capability)[bit >> 5]))
:-)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2016-01-20 16:20 +0100 |
| Subject | Re: [PATCH] x86: static_cpu_has_safe: discard dynamic check after init |
| Message-ID | <qT2Ic-8li-61@gated-at.bofh.it> |
| In reply to | #1313280 |
On 01/20/16 07:01, Borislav Petkov wrote: > On Tue, Jan 19, 2016 at 08:02:01PM -0800, H. Peter Anvin wrote: >> ... "i" (1 << (bit & 7)), >> "m" (((const char *)boot_cpu_data->x86_capability)[bit >> 3]) ... > > Nice! > > I was going to do: > > "i" (1 << (bit & 31)), > "m" (((boot_cpu_data->x86_capability)[bit >> 5])) > But then you're using testl and get long immediates. (And the parentheses around boot_cpu_data->x86_capability are redundant.) -hpa
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web