From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752733AbcADVB2 (ORCPT ); Mon, 4 Jan 2016 16:01:28 -0500 Received: from mail-db3on0067.outbound.protection.outlook.com ([157.55.234.67]:55032 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751908AbcADVBZ (ORCPT ); Mon, 4 Jan 2016 16:01:25 -0500 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=cmetcalf@ezchip.com; Subject: Re: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86 To: Mark Rutland References: <1451936091-29247-1-git-send-email-cmetcalf@ezchip.com> <1451936091-29247-9-git-send-email-cmetcalf@ezchip.com> <20160104203316.GE17127@leverpostej> CC: Gilad Ben Yossef , Steven Rostedt , Ingo Molnar , Peter Zijlstra , Andrew Morton , Rik van Riel , Tejun Heo , Frederic Weisbecker , Thomas Gleixner , "Paul E. McKenney" , Christoph Lameter , Viresh Kumar , Catalin Marinas , Will Deacon , Andy Lutomirski , , From: Chris Metcalf Message-ID: <568ADD91.1050100@ezchip.com> Date: Mon, 4 Jan 2016 16:01:05 -0500 User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160104203316.GE17127@leverpostej> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [12.216.194.146] X-ClientProxiedBy: CY1PR1201CA0014.namprd12.prod.outlook.com (2a01:111:e400:5b9a::24) To AM3PR02MB116.eurprd02.prod.outlook.com (2a01:111:e400:8807::19) X-Microsoft-Exchange-Diagnostics: 1;AM3PR02MB116;2:3URcwCHD7IzXwhZkVfOhF8ioBMJc3tTQUsD0riKfy2FvRa7nPgU0Lv63Vyuq4CwxGxNxl1iYEM8/Fsor77b7iGjXwYr1947lVkdV25o+kYLC1Y1AyvKXn3nzYSlkbm4oGAnmzqsZogG22JmsLzza+w==;3:Iq4L5J3mDN0CHIeuXTERrWEdNfgaBN+qhpN2Se0ZXEHSWbaWGL+vL1VVWe0q+PO1mCqIkVxkM+V+WYG3s5lwROQCBIa3Dt+k0ehi/PqpHtKAgm060X7HJtTEbRdZ+mzm;25:7StbzL9Mc5ao7o7/PeuBV6EBUJBEKpYL5zsBzwKu8zrrdB9iFf9ArFlodCysmUPyR8l4UVbbKp0QUfq+3RGj/b/peDoFwVb6bwGkvNAK0nOQKgwxrsEWDD81LarvDcxlDUfhxQ3ifDo4RL5aluXKEeoonVaoPcsTI0amXkp6SBqUifY8Ym6NdZuhKSmQeJrXFiDMc5iaH5Vwg1j7owPuYqImmYnqjYVpBXTG/NZuJZKSWqO94HzxRxFIyaBDWTdVw4ePqxyVt73PpnY8JdVHbw==;20:jAXn69NOkG/9etASbvWj0QskUVrJM4BJYvLS5/+n9+Kkx4X9WhSxXEww7vTeAwlhPgYxj/yBqL964XMlmCGI6gpkvuhUbTFThsIlnBmLmimP0K6y9cizgMlQCMGwz/NpXqUsGB6a2d1kFYz5GVJgzP7yNAUyINch95vy6AJQMVw= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM3PR02MB116; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(121898900299872); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(5005006)(8121501046)(10201501046)(3002001);SRVR:AM3PR02MB116;BCL:0;PCL:0;RULEID:;SRVR:AM3PR02MB116; X-Microsoft-Exchange-Diagnostics: 1;AM3PR02MB116;4:2AKsRbpUQsy3gQCfsKUIGNa9qXu7PXVJc7B3+fnAs2vjP5ev4OT+QVEUSnNGNO5zvTT67BchdzhIJxtkjwS91ti2W1+7VAnpTNMz7zgFL+TudWHkmDN7xry5C5zd3eHv4CQY0l/Wt5N+/wB9o//GJAQ9UYMUK4lQSPbLfHlsRkraLhZHUBPNsg1ZwUUXBL8rvCK50D2QI0lpOXMErJ/dViukiyOz24BLLfLcTwdEIZ7eDfEdMMfAdlJaGJzRG0BIiKax3+niJNB3yAgucnjTT7Ko9Zh+XW/yWKxEr/8Gi4KQ+0Ax2o8FswEdzrrDIwwjN6/zZxF7JKKk12E3MBp2ua6t8Z3i1RLhwn3VEJ3TfTaN6lz6HW7yRZWJjemydZfD81hzuY8+N05/BpeZDSupquYh6lgD1eXwZ2b8AV0a1QL4MrGveEPFgFckzf6frAaR X-Forefront-PRVS: 08118EFC2B X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6049001)(6009001)(164054003)(377454003)(24454002)(479174004)(199003)(43784003)(189002)(50986999)(106356001)(5001960100002)(101416001)(4001350100001)(97736004)(19580395003)(23746002)(36756003)(40100003)(230700001)(80316001)(189998001)(33656002)(5008740100001)(3846002)(105586002)(110136002)(122386002)(83506001)(59896002)(42186005)(77096005)(66066001)(6116002)(4326007)(87976001)(81156007)(92566002)(15975445007)(86362001)(2950100001)(586003)(65956001)(65816999)(5004730100002)(76176999)(65806001)(47776003)(50466002)(54356999)(64126003)(1096002)(87266999)(18886065003);DIR:OUT;SFP:1101;SCL:1;SRVR:AM3PR02MB116;H:[10.7.0.41];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;AM3PR02MB116;23:aChQGgLSKXn439KWC+bn/H34jOsA0KkA03T27L?= =?Windows-1252?Q?z3iL9zqznQOr0ZxN4iHPaiw6EtIoggCBQTxQX1XPDB0XhVc0TQg/YpcQ?= =?Windows-1252?Q?lzNYUI6TW+JNociBXvW0mYRHoxmqwKoM0vHNCDyA5d04pnCDMWm9GCxe?= =?Windows-1252?Q?hmr/L5PK2PDtuT5ZPX7+UkUs96zM7M4z/QQ27KlrkuoX/eC30DWpcRcV?= =?Windows-1252?Q?au5q+p0JSdZZYsv+xDuJgoHScc/SJpRxBpnR8hCDwMyk0URtexdmMt6f?= =?Windows-1252?Q?rQ2oLxMQ4S3N/Bp9gqpctzheG7bNNHLCpffcPHyZOeP2ZUj7OWA4IbYE?= =?Windows-1252?Q?5NFKD6ODMzPqrbdinKlEszZw3XZd83SKCu0txxTJazS76UJZyXsRQaCX?= =?Windows-1252?Q?r2aUpJA6GFOBy/VWascx7RrfipkIqaot6n2e+yC3sTdUI6+3dReicp+W?= =?Windows-1252?Q?GDvNYBhGROxv0isG0BBYXPsGrih19BWzC6b+10X4/Q3T+JYkJA/+K3vC?= =?Windows-1252?Q?6hxq3FhlaoI1peoha+XL87j3lQa6E+f/Tf8QznoO5oh/hiknnkuebNlH?= =?Windows-1252?Q?SCsGMOUkuGUoJ50TKAxGWtA3p4+78hg1wQXppwb3YKD9d2YVEbcoUPPR?= =?Windows-1252?Q?bUsI3YUE732aguWex64YogUQCA3bV9z35rFVoIsan3jtWoFznzZB8SFd?= =?Windows-1252?Q?GF8F8j8Mb3mfw9GZL8b0JLy4V1VGvgLVwm8uMORj5nAjkQUZFExGU6+G?= =?Windows-1252?Q?QLi27dz5lPh6cimlAr9gYcut0MSm3ulDlaKbk7oOFcuL0XGBkamsiGHE?= =?Windows-1252?Q?P3Kr9aIHp4hSiq+/Y0p9XQPspY50ymHFb6MCDUuwnuWEQHMzkWZq5C+f?= =?Windows-1252?Q?Sf4tNMruD6WtU5WKSyjaBupwd1JbAGH7RiPstikQF7VKzqzNW4bCAzBH?= =?Windows-1252?Q?f68mFRgq0Mi051ObNu9LjzcM/Cmv2Nm+PLvI3Q0ShEcG1Qg3yYSA0NXy?= =?Windows-1252?Q?xyPUW0mtCJEpyHJLf3iz77JSnFjaVqyLqstG81VoDcupQDQpXYdwdbm8?= =?Windows-1252?Q?C+SbEtBy6t7/vWbaaDatZigF2sxjNQfctiGD3kn51OUVA9BvlBjX7POM?= =?Windows-1252?Q?ipTWYifyyixok5ypwGjIDSwa+nSnNyus71wRRDvsjnbGxATNt+KpJQQq?= =?Windows-1252?Q?C2HmzqsnwZw8TZZMqEwKByw+rEWvKNCqiL7Xq4Wjsnr9F7Ch3j+i6O8X?= =?Windows-1252?Q?8kQKYo4E+IplpB6VL3wpZbghCYIHR8A/MUqNhVDnuI+ErNriCYLNCngk?= =?Windows-1252?Q?Ln9ibrgPPwE8vQ4/TnlKH2/Ej1XLDcpr0l2L+TbbbSWLPFTUVpjuO4eJ?= =?Windows-1252?Q?Cdk1c9w6IIM4pOeKamQPyhBHlUUCO5X0jJxjChZUG0w60A2sTGBmk13w?= =?Windows-1252?Q?RuoO6GN71IELOn5Me5Vt/cDfM4/T4prgGre5NwpVhRNHE43CN6qeFrMJ?= =?Windows-1252?Q?NEYnE=3D?= X-Microsoft-Exchange-Diagnostics: 1;AM3PR02MB116;5:/CyRwDYM1+sMtz6m386Bcr91wnnoiufvirYGzSW4LD6L58oi29mE/z6OZg0EZB/SvOZjejowI51j0RTIBFguVmoFy7DqGLEcUJzw4nEgl1zrJqdbc+XrkD5CNgM7/n343Len84s36h59gungsZx+HQ==;24:gru3Kel4nLlrvJgM4SyzEC1Sfdkj98S9HybcE9y4UuyG8eEx+HImv+vqoancYhADqKKWQJOVCCPROSNVy8Ly8rzs33QlxHWMsL9qpEtNcMA= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: ezchip.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2016 21:01:19.2297 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR02MB116 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: cmetcalf@ezchip.com (Chris Metcalf) Date: Mon, 4 Jan 2016 16:01:05 -0500 Subject: [PATCH v9 08/13] arch/arm64: adopt prepare_exit_to_usermode() model from x86 In-Reply-To: <20160104203316.GE17127@leverpostej> References: <1451936091-29247-1-git-send-email-cmetcalf@ezchip.com> <1451936091-29247-9-git-send-email-cmetcalf@ezchip.com> <20160104203316.GE17127@leverpostej> Message-ID: <568ADD91.1050100@ezchip.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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