All of lore.kernel.org
 help / color / mirror / Atom feed
From: "H. Peter Anvin" <hpa@zytor.com>
To: Andy Lutomirski <luto@amacapital.net>,
	Dave Hansen <dave.hansen@linux.intel.com>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Brian Gerst <brgerst@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit?
Date: Thu, 17 Dec 2015 22:43:21 -0800	[thread overview]
Message-ID: <FEA53E3B-6AE2-4305-82B1-C57451B6E33E@zytor.com> (raw)
In-Reply-To: <CALCETrWMofAX8TbZNyPaCWUHqcE85wpehDR05irEPtTYiVMXRA@mail.gmail.com>

On December 17, 2015 9:29:21 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Dec 17, 2015 6:53 PM, "Dave Hansen" <dave.hansen@linux.intel.com>
>wrote:
>>
>> On 12/17/2015 06:32 PM, Andy Lutomirski wrote:
>> > On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen
><dave.hansen@linux.intel.com> wrote:
>> >> But what about the register state when delivering a signal?  Don't
>we
>> >> set the registers to the init state?  Do we need to preserve PKRU
>state
>> >> instead of init'ing it?  The init state _is_ nice here because it
>is
>> >> permissive allows us to do something useful no matter what PKRU
>gets set to.
>> >
>> > I think we leave the extended regs alone.  Don't we?
>> >
>> > I think that, for most signals, we want to leave PKRU as is,
>> > especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we
>want
>> > an option to reset PKRU on delivery (and then set the flag to
>restore
>> > on return?).
>>
>> Is there some precedent for doing the state differently for different
>> signals?
>
>Yes, to a very limited extent: SA_ONSTACK.
>
>>
>> >> Well, the signal handler isn't necessarily going to clobber it,
>but the
>> >> delivery code already clobbers it when going to the init state.
>> >
>> > Can you point to that code?
>>
>> handle_signal() -> fpu__clear()
>>
>> The comment around it says:
>>
>> "Ensure the signal handler starts with the new fpu state."
>>
>
>You win this round :)
>
>So maybe we should have a mask of xfeatures that aren't cleared on
>signal delivery (e.g. PKRU, perhaps) and that are, by default,
>preserved across signal returns.
>
>> >>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures. 
>They
>> >>> appear to do more or less the same thing.
>> >>
>> >> I thought the _fpx_sw_bytes were only for 32-bit (or
>FXSAVE/FXRSTOR?).
>> >
>> > I thought they were everywhere.  fpu/signal.c looks that way to me.
> I
>> > could be missing something -- this code isn't the most
>straightforward
>> > in the world.
>>
>> I think there's some extra space on the ia32 frame for this stuff,
>but
>> some clarity from someone who knows the history would be appreciated.
>>
>> >> Not a huge deal, but something we want to think about, especially
>as it
>> >> pertains to the init/modified optimizations.
>> >
>> > Fair point.  FWIW, I don't think that sigreturn performance matters
>> > all that much, so if we inadvertently lose some of the
>optimizations,
>> > it may not be the end of the world.
>>
>> Once we lose the init optimization, we're sunk for good.  We never
>get
>> it back until we restore the init state again.  Having it in the init
>> state can save writing the state at XSAVE time, which can now save up
>to
>> ~2k of writes at each context switch.
>>
>> I'm sure we can preserve it, we just need to be _careful_.
>
>Right.
>
>How much does XSAVEOPT help here?  IOW if we're careful to save to the
>same place we restored from and we don't modify the state in the mean
>time, how much of the time do we save?  In the best case, I guess we
>save the memory writes but not the reads?
>
>--Andy

I find the notion of PKRU not being initialized at the entry to a signal handler a bit odd.  Saving/restoring it seems like the right thing to me.

What am I missing?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

  reply	other threads:[~2015-12-18  6:43 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-18  1:48 Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski
2015-12-18  2:13 ` Dave Hansen
2015-12-18  2:32   ` Andy Lutomirski
2015-12-18  2:52     ` Dave Hansen
2015-12-18  5:29       ` Andy Lutomirski
2015-12-18  6:43         ` H. Peter Anvin [this message]
2015-12-18 16:04           ` Andy Lutomirski
2015-12-18 16:56             ` Dave Hansen
2015-12-18 18:42             ` Dave Hansen
2015-12-18 19:21               ` Andy Lutomirski
2015-12-18 20:07                 ` Dave Hansen
2015-12-18 20:28                   ` Andy Lutomirski
2015-12-18 20:37                   ` Linus Torvalds
2015-12-18 20:49                     ` Andy Lutomirski
2015-12-18 20:58                       ` H. Peter Anvin
2015-12-18 21:02                         ` Andy Lutomirski
2015-12-18 21:08                           ` Dave Hansen
2015-12-18 21:04                       ` Linus Torvalds
2015-12-18 21:09                         ` Linus Torvalds
2015-12-18 21:12                         ` Dave Hansen
2015-12-18 21:45                           ` Linus Torvalds
2015-12-18 22:28                             ` Andy Lutomirski
2015-12-18 23:08                               ` Linus Torvalds
2015-12-18 23:16                                 ` Andy Lutomirski
2015-12-18 23:20                                   ` Linus Torvalds
2015-12-21 17:04                                   ` Dave Hansen
2015-12-21 22:52                                     ` Andy Lutomirski
2015-12-21 23:00                                       ` Dave Hansen
2015-12-21 23:02                                         ` Andy Lutomirski
2015-12-21 23:05                                           ` Dave Hansen
2015-12-21 23:04                               ` Dave Hansen
2015-12-21 23:07                                 ` Andy Lutomirski
2016-06-30 17:36                                   ` Andy Lutomirski
2016-06-30 21:25                                     ` Dave Hansen
2016-07-01 16:30                                       ` Andy Lutomirski
2015-12-29 23:48                             ` Dave Hansen
2015-12-18  8:32         ` Ingo Molnar
2015-12-18  8:59 ` Christoph Hellwig
2015-12-18 12:57   ` Borislav Petkov
2016-01-12 13:38     ` Ingo Molnar
2016-01-12 13:42       ` Christoph Hellwig
2016-01-13 10:48         ` Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=FEA53E3B-6AE2-4305-82B1-C57451B6E33E@zytor.com \
    --to=hpa@zytor.com \
    --cc=bp@alien8.de \
    --cc=brgerst@gmail.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.