From: Thomas Gleixner <tglx@linutronix.de> To: ira.weiny@intel.com, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Dave Hansen <dave.hansen@linux.intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com>, x86@kernel.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, linux-doc@vger.kernel.org, linux-nvdimm@lists.01.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Greg KH <gregkh@linuxfoundation.org> Subject: Re: [PATCH V3 04/10] x86/pks: Preserve the PKRS MSR on context switch Date: Thu, 17 Dec 2020 23:43:16 +0100 [thread overview] Message-ID: <87mtycqcjf.fsf@nanos.tec.linutronix.de> (raw) In-Reply-To: <871rfoscz4.fsf@nanos.tec.linutronix.de> On Thu, Dec 17 2020 at 15:50, Thomas Gleixner wrote: > On Fri, Nov 06 2020 at 15:29, ira weiny wrote: > >> +void write_pkrs(u32 new_pkrs) >> +{ >> + u32 *pkrs; >> + >> + if (!static_cpu_has(X86_FEATURE_PKS)) >> + return; >> + >> + pkrs = get_cpu_ptr(&pkrs_cache); > > So this is called from various places including schedule and also from > the low level entry/exit code. Why do we need to have an extra > preempt_disable/enable() there via get/put_cpu_ptr()? > > Just because performance in those code paths does not matter? > >> + if (*pkrs != new_pkrs) { >> + *pkrs = new_pkrs; >> + wrmsrl(MSR_IA32_PKRS, new_pkrs); >> + } >> + put_cpu_ptr(pkrs); Which made me look at the other branch of your git repo just because I wanted to know about the 'other' storage requirements and I found this gem: > update_global_pkrs() > ... > /* > * If we are preventing access from the old value. Force the > * update on all running threads. > */ > if (((old_val == 0) && protection) || > ((old_val & PKR_WD_BIT) && (protection & PKEY_DISABLE_ACCESS))) { > int cpu; > > for_each_online_cpu(cpu) { > u32 *ptr = per_cpu_ptr(&pkrs_cache, cpu); > > *ptr = update_pkey_val(*ptr, pkey, protection); > wrmsrl_on_cpu(cpu, MSR_IA32_PKRS, *ptr); > put_cpu_ptr(ptr); 1) per_cpu_ptr() -> put_cpu_ptr() is broken as per_cpu_ptr() is not disabling preemption while put_cpu_ptr() enables it which wreckages the preemption count. How was that ever tested at all with any debug option enabled? Answer: Not at all 2) How is that sequence: ptr = per_cpu_ptr(&pkrs_cache, cpu); *ptr = update_pkey_val(*ptr, pkey, protection); wrmsrl_on_cpu(cpu, MSR_IA32_PKRS, *ptr); supposed to be correct vs. a concurrent modification of the pkrs_cache of the remote CPU? Answer: Not at all Also doing a wrmsrl_on_cpu() on _each_ online CPU is insane at best. A smp function call on a remote CPU takes ~3-5us when the remote CPU is not idle and can immediately respond. If the remote CPU is deep in idle it can take up to 100us depending on C-State it is in. Even if the remote CPU is not not idle and just has interrupts disabled for a few dozen of microseconds this adds up. So on a 256 CPU system depending on the state of the remote CPUs this stalls the CPU doing the update for anything between 1 and 25ms worst case. Of course that also violates _all_ CPU isolation mechanisms. What for? Just for the theoretical chance that _all_ remote CPUs have seen that global permission and have it still active? You're not serious about that, right? The only use case for this in your tree is: kmap() and the possible usage of that mapping outside of the thread context which sets it up. The only hint for doing this at all is: Some users, such as kmap(), sometimes requires PKS to be global. 'sometime requires' is really _not_ a technical explanation. Where is the explanation why kmap() usage 'sometimes' requires this global trainwreck in the first place and where is the analysis why this can't be solved differently? Detailed use case analysis please. Thanks, tglx _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx@linutronix.de> To: ira.weiny@intel.com, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Dave Hansen <dave.hansen@linux.intel.com> Cc: Ira Weiny <ira.weiny@intel.com>, Fenghua Yu <fenghua.yu@intel.com>, x86@kernel.org, linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, linux-doc@vger.kernel.org, linux-nvdimm@lists.01.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Dan Williams <dan.j.williams@intel.com>, Greg KH <gregkh@linuxfoundation.org> Subject: Re: [PATCH V3 04/10] x86/pks: Preserve the PKRS MSR on context switch Date: Thu, 17 Dec 2020 23:43:16 +0100 [thread overview] Message-ID: <87mtycqcjf.fsf@nanos.tec.linutronix.de> (raw) In-Reply-To: <871rfoscz4.fsf@nanos.tec.linutronix.de> On Thu, Dec 17 2020 at 15:50, Thomas Gleixner wrote: > On Fri, Nov 06 2020 at 15:29, ira weiny wrote: > >> +void write_pkrs(u32 new_pkrs) >> +{ >> + u32 *pkrs; >> + >> + if (!static_cpu_has(X86_FEATURE_PKS)) >> + return; >> + >> + pkrs = get_cpu_ptr(&pkrs_cache); > > So this is called from various places including schedule and also from > the low level entry/exit code. Why do we need to have an extra > preempt_disable/enable() there via get/put_cpu_ptr()? > > Just because performance in those code paths does not matter? > >> + if (*pkrs != new_pkrs) { >> + *pkrs = new_pkrs; >> + wrmsrl(MSR_IA32_PKRS, new_pkrs); >> + } >> + put_cpu_ptr(pkrs); Which made me look at the other branch of your git repo just because I wanted to know about the 'other' storage requirements and I found this gem: > update_global_pkrs() > ... > /* > * If we are preventing access from the old value. Force the > * update on all running threads. > */ > if (((old_val == 0) && protection) || > ((old_val & PKR_WD_BIT) && (protection & PKEY_DISABLE_ACCESS))) { > int cpu; > > for_each_online_cpu(cpu) { > u32 *ptr = per_cpu_ptr(&pkrs_cache, cpu); > > *ptr = update_pkey_val(*ptr, pkey, protection); > wrmsrl_on_cpu(cpu, MSR_IA32_PKRS, *ptr); > put_cpu_ptr(ptr); 1) per_cpu_ptr() -> put_cpu_ptr() is broken as per_cpu_ptr() is not disabling preemption while put_cpu_ptr() enables it which wreckages the preemption count. How was that ever tested at all with any debug option enabled? Answer: Not at all 2) How is that sequence: ptr = per_cpu_ptr(&pkrs_cache, cpu); *ptr = update_pkey_val(*ptr, pkey, protection); wrmsrl_on_cpu(cpu, MSR_IA32_PKRS, *ptr); supposed to be correct vs. a concurrent modification of the pkrs_cache of the remote CPU? Answer: Not at all Also doing a wrmsrl_on_cpu() on _each_ online CPU is insane at best. A smp function call on a remote CPU takes ~3-5us when the remote CPU is not idle and can immediately respond. If the remote CPU is deep in idle it can take up to 100us depending on C-State it is in. Even if the remote CPU is not not idle and just has interrupts disabled for a few dozen of microseconds this adds up. So on a 256 CPU system depending on the state of the remote CPUs this stalls the CPU doing the update for anything between 1 and 25ms worst case. Of course that also violates _all_ CPU isolation mechanisms. What for? Just for the theoretical chance that _all_ remote CPUs have seen that global permission and have it still active? You're not serious about that, right? The only use case for this in your tree is: kmap() and the possible usage of that mapping outside of the thread context which sets it up. The only hint for doing this at all is: Some users, such as kmap(), sometimes requires PKS to be global. 'sometime requires' is really _not_ a technical explanation. Where is the explanation why kmap() usage 'sometimes' requires this global trainwreck in the first place and where is the analysis why this can't be solved differently? Detailed use case analysis please. Thanks, tglx
next prev parent reply other threads:[~2020-12-17 22:43 UTC|newest] Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-06 23:28 [PATCH V3 00/10] PKS: Add Protection Keys Supervisor (PKS) support V3 ira.weiny 2020-11-06 23:28 ` ira.weiny 2020-11-06 23:28 ` [PATCH V3 01/10] x86/pkeys: Create pkeys_common.h ira.weiny 2020-11-06 23:28 ` ira.weiny 2020-11-06 23:29 ` [PATCH V3 02/10] x86/fpu: Refactor arch_set_user_pkey_access() for PKS support ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-11-06 23:29 ` [PATCH V3 03/10] x86/pks: Add PKS defines and Kconfig options ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-11-06 23:29 ` [PATCH V3 04/10] x86/pks: Preserve the PKRS MSR on context switch ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-12-17 14:50 ` Thomas Gleixner 2020-12-17 14:50 ` Thomas Gleixner 2020-12-17 22:43 ` Thomas Gleixner [this message] 2020-12-17 22:43 ` Thomas Gleixner 2020-12-18 13:57 ` Thomas Gleixner 2020-12-18 13:57 ` Thomas Gleixner 2020-12-18 19:20 ` Dan Williams 2020-12-18 19:20 ` Dan Williams 2020-12-18 19:20 ` Dan Williams 2020-12-18 21:06 ` Thomas Gleixner 2020-12-18 21:06 ` Thomas Gleixner 2020-12-18 21:58 ` Dan Williams 2020-12-18 21:58 ` Dan Williams 2020-12-18 21:58 ` Dan Williams 2020-12-18 22:44 ` Thomas Gleixner 2020-12-18 22:44 ` Thomas Gleixner 2020-12-18 19:42 ` Ira Weiny 2020-12-18 19:42 ` Ira Weiny 2020-12-18 20:10 ` Dave Hansen 2020-12-18 20:10 ` Dave Hansen 2020-12-18 21:30 ` Thomas Gleixner 2020-12-18 21:30 ` Thomas Gleixner 2020-12-18 4:05 ` Ira Weiny 2020-12-18 4:05 ` Ira Weiny 2020-12-17 20:41 ` [NEEDS-REVIEW] " Dave Hansen 2020-12-17 20:41 ` Dave Hansen 2020-12-18 4:10 ` Ira Weiny 2020-12-18 4:10 ` Ira Weiny 2020-12-18 15:33 ` Dave Hansen 2020-12-18 15:33 ` Dave Hansen 2020-11-06 23:29 ` [PATCH V3 05/10] x86/entry: Pass irqentry_state_t by reference ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-11-15 18:58 ` Thomas Gleixner 2020-11-15 18:58 ` Thomas Gleixner 2020-11-16 18:49 ` Ira Weiny 2020-11-16 18:49 ` Ira Weiny 2020-11-16 20:36 ` Thomas Gleixner 2020-11-16 20:36 ` Thomas Gleixner 2020-11-24 6:09 ` [PATCH V3.1] entry: " ira.weiny 2020-11-24 6:09 ` ira.weiny 2020-12-11 22:14 ` Andy Lutomirski 2020-12-11 22:14 ` Andy Lutomirski 2020-12-11 22:14 ` Andy Lutomirski 2020-12-16 1:32 ` Ira Weiny 2020-12-16 1:32 ` Ira Weiny 2020-12-16 1:32 ` Ira Weiny 2020-12-16 2:09 ` Andy Lutomirski 2020-12-16 2:09 ` Andy Lutomirski 2020-12-16 2:09 ` Andy Lutomirski 2020-12-17 0:38 ` Ira Weiny 2020-12-17 0:38 ` Ira Weiny 2020-12-17 0:38 ` Ira Weiny 2020-12-17 13:07 ` Thomas Gleixner 2020-12-17 13:07 ` Thomas Gleixner 2020-12-17 13:07 ` Thomas Gleixner 2020-12-17 13:19 ` Peter Zijlstra 2020-12-17 13:19 ` Peter Zijlstra 2020-12-17 13:19 ` Peter Zijlstra 2020-12-17 15:35 ` Andy Lutomirski 2020-12-17 15:35 ` Andy Lutomirski 2020-12-17 15:35 ` Andy Lutomirski 2020-12-17 16:58 ` Thomas Gleixner 2020-12-17 16:58 ` Thomas Gleixner 2020-11-06 23:29 ` [PATCH V3 06/10] x86/entry: Preserve PKRS MSR across exceptions ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-12-17 15:28 ` Thomas Gleixner 2020-12-17 15:28 ` Thomas Gleixner 2020-11-06 23:29 ` [PATCH V3 07/10] x86/fault: Report the PKRS state on fault ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-11-06 23:29 ` [PATCH V3 08/10] x86/pks: Add PKS kernel API ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-12-23 20:39 ` Randy Dunlap 2020-12-23 20:39 ` Randy Dunlap 2020-11-06 23:29 ` [PATCH V3 09/10] x86/pks: Enable Protection Keys Supervisor (PKS) ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-11-06 23:29 ` [PATCH V3 10/10] x86/pks: Add PKS test code ira.weiny 2020-11-06 23:29 ` ira.weiny 2020-12-17 20:55 ` Dave Hansen 2020-12-17 20:55 ` Dave Hansen 2020-12-18 4:05 ` Ira Weiny 2020-12-18 4:05 ` Ira Weiny 2020-12-18 16:59 ` Dan Williams 2020-12-18 16:59 ` Dan Williams 2020-12-18 16:59 ` Dan Williams 2020-12-07 22:14 ` [PATCH V3 00/10] PKS: Add Protection Keys Supervisor (PKS) support V3 Ira Weiny 2020-12-07 22:14 ` Ira Weiny 2020-12-08 15:55 ` Thomas Gleixner 2020-12-08 15:55 ` Thomas Gleixner 2020-12-08 17:22 ` Ira Weiny 2020-12-08 17:22 ` Ira Weiny
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=87mtycqcjf.fsf@nanos.tec.linutronix.de \ --to=tglx@linutronix.de \ --cc=akpm@linux-foundation.org \ --cc=bp@alien8.de \ --cc=dave.hansen@linux.intel.com \ --cc=fenghua.yu@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=ira.weiny@intel.com \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nvdimm@lists.01.org \ --cc=luto@kernel.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=x86@kernel.org \ /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: linkBe 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.