From: cmetcalf@ezchip•com (Chris Metcalf)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86
Date: Mon, 4 Jan 2016 16:01:05 -0500 [thread overview]
Message-ID: <568ADD91.1050100@ezchip.com> (raw)
In-Reply-To: <20160104203316.GE17127@leverpostej>
On 01/04/2016 03:33 PM, Mark Rutland wrote:
> Hi,
>
> On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
>> This change is a prerequisite change for TASK_ISOLATION but also
>> stands on its own for readability and maintainability.
> I have also been looking into converting the userspace return path from
> assembly to C [1], for the latter two reasons. Based on that, I have a
> couple of comments.
Thanks!
> It seems unfortunate to leave behind portions of the entry.S
> _TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
> and the majority of work_pending and ret_to_user).
>
> I think it would be nicer if we could handle all of that in one place
> (or at least all in C).
Yes, in principle I agree with this, and I think your deasm tree looks
like an excellent idea.
For this patch series I wanted to focus more on what was necessary
for the various platforms to implement task isolation, and less on
additional cleanups of the platforms in question. I think my changes
don't make the TIF state machine any less clear, nor do they make
it harder for an eventual further migration to C code along the lines
of what you've done, so it seems plausible to me to commit them
upstream independently of your work.
>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index e18c48cb6db1..fde59c1139a9 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -399,18 +399,30 @@ static void do_signal(struct pt_regs *regs)
>> restore_saved_sigmask();
>> }
>>
>> -asmlinkage void do_notify_resume(struct pt_regs *regs,
>> - unsigned int thread_flags)
>> +asmlinkage void prepare_exit_to_usermode(struct pt_regs *regs,
>> + unsigned int thread_flags)
>> {
>> - if (thread_flags & _TIF_SIGPENDING)
>> - do_signal(regs);
>> + do {
>> + local_irq_enable();
>>
>> - if (thread_flags & _TIF_NOTIFY_RESUME) {
>> - clear_thread_flag(TIF_NOTIFY_RESUME);
>> - tracehook_notify_resume(regs);
>> - }
>> + if (thread_flags & _TIF_NEED_RESCHED)
>> + schedule();
> Previously, had we called schedule(), we'd reload the thread info flags
> and start that state machine again, whereas now we'll handle all the
> cached flags before reloading.
>
> Are we sure nothing is relying on the prior behaviour?
Good eye, and I probably should have called that out in the commit
message. My best guess is that there should be nothing that depends
on the old semantics. Other platforms (certainly x86 and tile, anyway)
already have the semantics that you run out the old state machine on
return from schedule(), so regardless, it's probably appropriate for
arm to follow that same convention.
>> +
>> + if (thread_flags & _TIF_SIGPENDING)
>> + do_signal(regs);
>> +
>> + if (thread_flags & _TIF_NOTIFY_RESUME) {
>> + clear_thread_flag(TIF_NOTIFY_RESUME);
>> + tracehook_notify_resume(regs);
>> + }
>> +
>> + if (thread_flags & _TIF_FOREIGN_FPSTATE)
>> + fpsimd_restore_current_state();
>> +
>> + local_irq_disable();
>>
>> - if (thread_flags & _TIF_FOREIGN_FPSTATE)
>> - fpsimd_restore_current_state();
>> + thread_flags = READ_ONCE(current_thread_info()->flags) &
>> + _TIF_WORK_MASK;
>>
>> + } while (thread_flags);
>> }
> Other than that, this looks good to me.
>
> Thanks,
> Mark.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=arm64/entry-deasm
Thanks again for the review - shall I add your Reviewed-by (or Acked-by?)
to this patch?
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
next prev parent reply other threads:[~2016-01-04 21:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1451936091-29247-1-git-send-email-cmetcalf@ezchip.com>
2016-01-04 19:34 ` [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86 Chris Metcalf
2016-01-04 20:33 ` Mark Rutland
2016-01-04 21:01 ` Chris Metcalf [this message]
2016-01-05 17:21 ` Mark Rutland
2016-01-05 17:33 ` [PATCH 1/2] arm64: entry: remove pointless SPSR mode check Mark Rutland
2016-01-06 12:15 ` Catalin Marinas
2016-01-05 17:33 ` [PATCH 2/2] arm64: factor work_pending state machine to C Mark Rutland
2016-01-05 18:53 ` Chris Metcalf
2016-01-06 12:30 ` Catalin Marinas
2016-01-06 12:47 ` Mark Rutland
2016-01-06 13:43 ` Mark Rutland
2016-01-06 14:17 ` Catalin Marinas
2016-01-04 22:31 ` [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86 Andy Lutomirski
2016-01-05 18:01 ` Mark Rutland
2016-01-04 19:34 ` [PATCH v9 09/13] arch/arm64: enable task isolation functionality Chris Metcalf
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=568ADD91.1050100@ezchip.com \
--to=cmetcalf@ezchip$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox