linux-kernel - Re: RFC: Petition Intel/AMD to add POPF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Aug 2016 14:26:51 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Denys Vlasenko <dvlasenk@...hat.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	Sara Sharon <sara.sharon@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Christian König <christian.koenig@....com>,
	Vinod Koul <vinod.koul@...el.com>,
	Alex Deucher <alexander.deucher@....com>,
	Johannes Berg <johannes.berg@...el.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Andy Lutomirski <luto@...nel.org>,
	"the arch/x86 maintainers" <x86@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: RFC: Petition Intel/AMD to add POPF_IF insn

On Wed, Aug 17, 2016 at 12:37 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> Replace the "popf" with "if (val & X86_EFLAGS_IF) local_irq_enable();"
> and see how that works out. Play with inlining it or not, and see if
> the branch predictor matters.

.. actually, thinking a bit more about it, I really don't think the
branch predictor will even matter.

We sure as hell shouldn't have nested irq-safe interrupts in paths
that matter from a performance angle, so the normal case for
spin_unlock_irqrestore() should be to enable interrupts again.

And if interrupts are disabled because the caller is actually in
interrupt context, I don't think the branch prediction is going to
matter, compared to the irq overhead.

So test this trivial patch. It's ENTIRELY UNTESTED. It may be complete
crap and not even compile. But I did test it on
kernel/locking/spinlock.c, and the generated code is beautiful:

  _raw_spin_unlock_irqrestore:
        testl   $512, %esi      #, flags
        movb    $0, (%rdi)      #, MEM[(volatile __u8 *)lock_2(D)]
        je      .L2
        sti
  .L2:
        ret

so maybe the silly popf has always just been stupid.

Of course, if somebody uses native_restore_fl() to actually *disable*
interrupts (when they weren't already disabled), then this untested
patch will just not work. But why would you do somethign so stupid?
Famous last words...

                 Linus

View attachment "patch.diff" of type "text/plain" (1127 bytes)