Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

[Andy Lutomirski]

On Mon, Feb 29, 2016 at 12:39 PM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Sun, Feb 28, 2016 at 09:28:47PM -0800, Andy Lutomirski wrote:
>> CLAC is slow, and the SYSENTER code already has an unlikely path
>> that runs if unusual flags are set.  Drop the CLAC and instead rely
>> on the unlikely path to clear AC.
>>
>> This seems to save ~24 cycles on my Skylake laptop.  (Hey, Intel,
>> make this faster please!)
>>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>>  arch/x86/entry/entry_64_compat.S | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
>> index 89bcb4979e7a..7c8e72da7654 100644
>> --- a/arch/x86/entry/entry_64_compat.S
>> +++ b/arch/x86/entry/entry_64_compat.S
>> @@ -66,8 +66,6 @@ ENTRY(entry_SYSENTER_compat)
>>        */
>>       pushfq                          /* pt_regs->flags (except IF = 0) */
>>       orl     $X86_EFLAGS_IF, (%rsp)  /* Fix saved flags */
>> -     ASM_CLAC                        /* Clear AC after saving FLAGS */
>> -
>>       pushq   $__USER32_CS            /* pt_regs->cs */
>>       xorq    %r8,%r8
>>       pushq   %r8                     /* pt_regs->ip = 0 (placeholder) */
>> @@ -90,9 +88,9 @@ ENTRY(entry_SYSENTER_compat)
>>       cld
>>
>>       /*
>> -      * Sysenter doesn't filter flags, so we need to clear NT
>> +      * Sysenter doesn't filter flags, so we need to clear NT and AC
>>        * ourselves.  To save a few cycles, we can check whether
>> -      * NT was set instead of doing an unconditional popfq.
>> +      * either was set instead of doing an unconditional popfq.
>>        * This needs to happen before enabling interrupts so that
>>        * we don't get preempted with NT set.
>>        *
>> @@ -102,7 +100,7 @@ ENTRY(entry_SYSENTER_compat)
>>        * we're keeping that code behind a branch which will predict as
>>        * not-taken and therefore its instructions won't be fetched.
>>        */
>> -     testl   $X86_EFLAGS_NT, EFLAGS(%rsp)
>> +     testl   $X86_EFLAGS_NT|X86_EFLAGS_AC, EFLAGS(%rsp)
>>       jnz     .Lsysenter_fix_flags
>>  .Lsysenter_flags_fixed:
>
> Do I see it correctly that with this change, that .Lsysenter_fix_flags:
> is going to be visited each time on SMAP machines and then we can get
> rid of it? The reason for it was not to pollute I$ as the comment says
> but that happening now anyway...
>

I don't think so.  Sensible user programs shouldn't set AC in the first place.

--Andy