* Re: [PATCH v8 15/27] mm: Handle shadow stack page fault
From: Yu-cheng Yu @ 2019-08-14 17:00 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <bf8a6390-97a6-1ab6-90ef-6399437ed38c@intel.com>
On Wed, 2019-08-14 at 09:48 -0700, Dave Hansen wrote:
> On 8/14/19 9:27 AM, Yu-cheng Yu wrote:
> > On Tue, 2019-08-13 at 15:55 -0700, Andy Lutomirski wrote:
> > > On Tue, Aug 13, 2019 at 2:02 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > > When a task does fork(), its shadow stack (SHSTK) must be duplicated
> > > > for the child. This patch implements a flow similar to copy-on-write
> > > > of an anonymous page, but for SHSTK.
> > > >
> > > > A SHSTK PTE must be RO and dirty. This dirty bit requirement is used
> > > > to effect the copying. In copy_one_pte(), clear the dirty bit from a
> > > > SHSTK PTE to cause a page fault upon the next SHSTK access. At that
> > > > time, fix the PTE and copy/re-use the page.
> > >
> > > Is using VM_SHSTK and special-casing all of this really better than
> > > using a special mapping or other pseudo-file-backed VMA and putting
> > > all the magic in the vm_operations?
> >
> > A special mapping is cleaner. However, we also need to exclude normal [RO +
> > dirty] pages from shadow stack.
>
> I don't understand what you are saying.
>
> Are you saying that we need this VM_SHSTK flag in order to exclude
> RO+HW-Dirty pages from being created in non-shadow-stack VMAs?
We use VM_SHSTK for page fault handling (the special-casing). If we have a
special mapping, all these become cleaner (but more code). However, we still
need most of the PTE macros (e.g. ptep_set_wrprotect, PAGE_DIRTY_SW, etc.).
Yu-cheng
^ permalink raw reply
* Re: [RFC 06/19] ktf: A simple debugfs interface to test results
From: Knut Omang @ 2019-08-14 17:17 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kselftest, linux-kernel, linux-doc, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <20190813082152.GA17627@kroah.com>
On Tue, 2019-08-13 at 10:21 +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2019 at 08:09:21AM +0200, Knut Omang wrote:
> > From: Alan Maguire <alan.maguire@oracle.com>
> >
> > While test results is available via netlink from user space, sometimes
> > it may be useful to be able to access the results from the kernel as well,
> > for instance due to a crash. Make that possible via debugfs.
> >
> > ktf_debugfs.h: Support for creating a debugfs representation of test
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> > tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++++++++++++++-
> > tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 ++-
> > 2 files changed, 390 insertions(+)
> > create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> > create mode 100644 tools/testing/selftests/ktf/kernel/ktf_debugfs.h
> >
> > diff --git a/tools/testing/selftests/ktf/kernel/ktf_debugfs.c b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> > new file mode 100644
> > index 0000000..a20fbd2
> > --- /dev/null
> > +++ b/tools/testing/selftests/ktf/kernel/ktf_debugfs.c
> > @@ -0,0 +1,356 @@
> > +/*
> > + * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
> > + * Author: Alan Maguire <alan.maguire@oracle.com>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
>
> Has to be the first line of the file, did you run this through
> checkpatch?
Yes, the code has been subject to continuous integration which uses
a version of my runchecks tool (https://lkml.org/lkml/2018/1/19/157)
to ensure that it is not possible to "forget" to run checkpatch
(or sparse, smatch doc.check for that sake)
Ironically though I fell victim to my own tooling here,
as I postponed fixing the SPDX_LICENSE_TAG class of issues
once that test appeared, while working on something else,
and just forgot to re-enable it again..
> > +static int ktf_run_test_open(struct inode *inode, struct file *file)
> > +{
> > + struct ktf_test *t;
> > +
> > + if (!try_module_get(THIS_MODULE))
> > + return -EIO;
>
> This is an anti-pattern, and one guaranteed to not work properly. NEVER
> do this.
Sorry, I didn't know this, and the origin is probably my responsibility.
I know the feeling of never being able to get rid of bad examples
because they keep getting copied..
The pattern seemed to be widely used the first time I saw it, and although
somewhat awkward, it seemed to be the standard way then, but as you know,
my Infiniband driver (
https://github.com/oracle/linux-uek/blob/uek4/qu7/drivers/infiniband/hw/sif/sif_debug.c)
unfortunately never made it to the scrutiny of LKML, since the hardware project
got cancelled.
The -EIO return value was also copied from merged kernel code back then.
I notice the discussion and your response here:
http://linux-kernel.2935.n7.nabble.com/debugfs-and-module-unloading-td865175.html
I assume that means that protection against module unload while a debugfs file
is open is now safe.
On older kernels, having this code in place is far better than an unprotected
debugfs entry/exit - I have tested it extensively in the past :-)
Back when I first used it, I had this cool set of polymorphic
debugfs file code to list the set of active MRs, CQs, QPs, AHs etc
that the whole infiniband driver, database and hardware teams loved
so much that multiple users ended up using it in multiple windows
from within watch for live observations of state changes,
and often also running driver load/unloads for testing purposes.
I perfectly agree with you that reducing the hole for a race condition
is generally a bad idea, but from the above mail thread
it seems that's the only available choice for older kernels?
(I am asking because I still want to be able to support rather
old kernels with the github version of KTF)
Anyway, great to know that a better solution now exists!
We'll fix the rest of the issues below as well for the next version..
Thanks!
Knut
> > +
> > + t = (struct ktf_test *)inode->i_private;
> > +
> > + return single_open(file, ktf_debugfs_run, t);
> > +}
> > +
> > +static int ktf_debugfs_release(struct inode *inode, struct file *file)
> > +{
> > + module_put(THIS_MODULE);
>
> Same here, not ok.
>
>
> > + return single_release(inode, file);
> > +}
> > +
> > +static const struct file_operations ktf_run_test_fops = {
> > + .open = ktf_run_test_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = ktf_debugfs_release,
> > +};
> > +
> > +static int ktf_results_test_open(struct inode *inode, struct file *file)
> > +{
> > + struct ktf_test *t;
> > +
> > + if (!try_module_get(THIS_MODULE))
> > + return -EIO;
>
> Nope!
>
> And why -EIO? That is not an io issue.
Agreed
>
> > +void ktf_debugfs_create_test(struct ktf_test *t)
> > +{
> > + struct ktf_case *testset = ktf_case_find(t->tclass);
> > +
> > + if (!testset)
> > + return;
> > +
> > + memset(&t->debugfs, 0, sizeof(t->debugfs));
> > +
> > + t->debugfs.debugfs_results_test =
> > + debugfs_create_file(t->name, S_IFREG | 0444,
> > + testset->debugfs.debugfs_results_test,
> > + t, &ktf_results_test_fops);
> > +
> > + if (t->debugfs.debugfs_results_test) {
>
> How can that variable ever be NULL (hint, it can not.)
>
> > + t->debugfs.debugfs_run_test =
> > + debugfs_create_file(t->name, S_IFREG | 0444,
> > + testset->debugfs.debugfs_run_test,
> > + t, &ktf_run_test_fops);
> > + if (!t->debugfs.debugfs_run_test) {
> > + _ktf_debugfs_destroy_test(t);
> > + } else {
> > + /* Take reference for test for debugfs */
> > + ktf_test_get(t);
> > + }
> > + }
>
> Never test the result of any debugfs call, you do not need to. Just
> call it and move on, your code flow should NEVER be different with, or
> without, a successful debugfs call.
>
>
> > +static int ktf_run_testset_open(struct inode *inode, struct file *file)
> > +{
> > + struct ktf_case *testset;
> > +
> > + if (!try_module_get(THIS_MODULE))
> > + return -EIO;
>
> Again no. I hate to know what code you copied this all from, as that
> code is very wrong. Do you have a pointer to that code anywhere so we
> can fix that up?
>
> > +
> > + testset = (struct ktf_case *)inode->i_private;
> > +
> > + return single_open(file, ktf_debugfs_run_all, testset);
> > +}
> > +
> > +static const struct file_operations ktf_run_testset_fops = {
> > + .open = ktf_run_testset_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = ktf_debugfs_release,
>
> If you really care about module references you should be setting the
> owner of the module here.
>
> > +};
> > +
> > +static void _ktf_debugfs_destroy_testset(struct ktf_case *testset)
> > +{
> > + debugfs_remove(testset->debugfs.debugfs_run_testset);
> > + debugfs_remove(testset->debugfs.debugfs_run_test);
> > + debugfs_remove(testset->debugfs.debugfs_results_testset);
> > + debugfs_remove(testset->debugfs.debugfs_results_test);
>
> Why not just recursivly remove the directory? That way you do not have
> to keep track of any individual files.
>
>
> > +}
> > +
> > +void ktf_debugfs_create_testset(struct ktf_case *testset)
> > +{
> > + char tests_subdir[KTF_DEBUGFS_NAMESZ];
> > + const char *name = ktf_case_name(testset);
> > +
> > + memset(&testset->debugfs, 0, sizeof(testset->debugfs));
> > +
> > + /* First add /sys/kernel/debug/ktf/[results|run]/<testset> */
> > + testset->debugfs.debugfs_results_testset =
> > + debugfs_create_file(name, S_IFREG | 0444,
> > + ktf_debugfs_resultsdir,
> > + testset, &ktf_results_testset_fops);
> > + if (!testset->debugfs.debugfs_results_testset)
> > + goto err;
>
> Again, can never happen, and again, do not do different things depending
> on the result of a debugfs call.
>
> > +
> > + testset->debugfs.debugfs_run_testset =
> > + debugfs_create_file(name, S_IFREG | 0444,
> > + ktf_debugfs_rundir,
> > + testset, &ktf_run_testset_fops);
> > + if (!testset->debugfs.debugfs_run_testset)
> > + goto err;
>
> Again, nope.
>
> > +
> > + /* Now add parent directories for individual test result/run tests
> > + * which live in
> > + * /sys/kernel/debug/ktf/[results|run]/<testset>-tests/<testname>
> > + */
> > + (void)snprintf(tests_subdir, sizeof(tests_subdir), "%s%s",
> > + name, KTF_DEBUGFS_TESTS_SUFFIX);
>
> why (void)?
>
>
> > +
> > + testset->debugfs.debugfs_results_test =
> > + debugfs_create_dir(tests_subdir, ktf_debugfs_resultsdir);
> > + if (!testset->debugfs.debugfs_results_test)
> > + goto err;
>
> nope :)
>
> > +
> > + testset->debugfs.debugfs_run_test =
> > + debugfs_create_dir(tests_subdir, ktf_debugfs_rundir);
> > + if (!testset->debugfs.debugfs_run_test)
> > + goto err;
>
> Nope :)
>
> > +
> > + /* Take reference count for testset. One will do as we will always
> > + * free testset debugfs resources together.
> > + */
> > + ktf_case_get(testset);
> > + return;
> > +err:
> > + _ktf_debugfs_destroy_testset(testset);
> > +}
> > +
> > +void ktf_debugfs_destroy_testset(struct ktf_case *testset)
> > +{
> > + tlog(T_DEBUG, "Destroying debugfs testset %s", ktf_case_name(testset));
> > + _ktf_debugfs_destroy_testset(testset);
> > + /* Remove our debugfs reference cout to testset */
> > + ktf_case_put(testset);
> > +}
> > +
> > +/* /sys/kernel/debug/ktf/coverage shows coverage statistics. */
> > +static int ktf_debugfs_cov(struct seq_file *seq, void *v)
> > +{
> > + ktf_cov_seq_print(seq);
> > +
> > + return 0;
> > +}
> > +
> > +static int ktf_cov_open(struct inode *inode, struct file *file)
> > +{
> > + if (!try_module_get(THIS_MODULE))
> > + return -EIO;
>
> {sigh} I'll stop reviewing now :)
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Dave Hansen @ 2019-08-14 16:58 UTC (permalink / raw)
To: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra, Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue,
Dave Martin
In-Reply-To: <c7731c682b55ec882ad3d4ea11ad7a823dcaae8f.camel@intel.com>
On 8/14/19 9:42 AM, Yu-cheng Yu wrote:
> On Tue, 2019-08-13 at 16:02 -0700, Dave Hansen wrote:
> [...]
>> Please also reconcile the supervisor XSAVE portion of your patches with
>> the ones that Fenghua has been sending around. I've given quite a bit
>> of feedback to improve those. Please consolidate and agree on a common
>> set of patches with him.
> XSAVES supervisor is now a six-patch set. Maybe we can make it a separate
> series? I will consolidate and send it out.
A separate series would be great.
Please also make sure it's in a (temporary) git tree somewhere so that
it's easy to base other sets on top of it.
^ permalink raw reply
* Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Yu-cheng Yu @ 2019-08-14 16:42 UTC (permalink / raw)
To: Dave Hansen, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra, Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue,
Dave Martin
In-Reply-To: <dac2d62b-9045-4767-87dd-eac12e7abafd@intel.com>
On Tue, 2019-08-13 at 16:02 -0700, Dave Hansen wrote:
[...]
> Please also reconcile the supervisor XSAVE portion of your patches with
> the ones that Fenghua has been sending around. I've given quite a bit
> of feedback to improve those. Please consolidate and agree on a common
> set of patches with him.
XSAVES supervisor is now a six-patch set. Maybe we can make it a separate
series? I will consolidate and send it out.
Yu-cheng
^ permalink raw reply
* Re: [PATCH v8 15/27] mm: Handle shadow stack page fault
From: Dave Hansen @ 2019-08-14 16:48 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <eabd0c16bd2028ad9fef8d10ddf570b3a10d5680.camel@intel.com>
On 8/14/19 9:27 AM, Yu-cheng Yu wrote:
> On Tue, 2019-08-13 at 15:55 -0700, Andy Lutomirski wrote:
>> On Tue, Aug 13, 2019 at 2:02 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>>> When a task does fork(), its shadow stack (SHSTK) must be duplicated
>>> for the child. This patch implements a flow similar to copy-on-write
>>> of an anonymous page, but for SHSTK.
>>>
>>> A SHSTK PTE must be RO and dirty. This dirty bit requirement is used
>>> to effect the copying. In copy_one_pte(), clear the dirty bit from a
>>> SHSTK PTE to cause a page fault upon the next SHSTK access. At that
>>> time, fix the PTE and copy/re-use the page.
>> Is using VM_SHSTK and special-casing all of this really better than
>> using a special mapping or other pseudo-file-backed VMA and putting
>> all the magic in the vm_operations?
> A special mapping is cleaner. However, we also need to exclude normal [RO +
> dirty] pages from shadow stack.
I don't understand what you are saying.
Are you saying that we need this VM_SHSTK flag in order to exclude
RO+HW-Dirty pages from being created in non-shadow-stack VMAs?
^ permalink raw reply
* Re: [PATCH v8 15/27] mm: Handle shadow stack page fault
From: Yu-cheng Yu @ 2019-08-14 16:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <CALCETrVKbqzivPfUOiGi5efHUpEsfPkNzP0CrmAZzcwUgf7quA@mail.gmail.com>
On Tue, 2019-08-13 at 15:55 -0700, Andy Lutomirski wrote:
> On Tue, Aug 13, 2019 at 2:02 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > When a task does fork(), its shadow stack (SHSTK) must be duplicated
> > for the child. This patch implements a flow similar to copy-on-write
> > of an anonymous page, but for SHSTK.
> >
> > A SHSTK PTE must be RO and dirty. This dirty bit requirement is used
> > to effect the copying. In copy_one_pte(), clear the dirty bit from a
> > SHSTK PTE to cause a page fault upon the next SHSTK access. At that
> > time, fix the PTE and copy/re-use the page.
>
> Is using VM_SHSTK and special-casing all of this really better than
> using a special mapping or other pseudo-file-backed VMA and putting
> all the magic in the vm_operations?
A special mapping is cleaner. However, we also need to exclude normal [RO +
dirty] pages from shadow stack.
Yu-cheng
^ permalink raw reply
* Re: [PATCH v3 01/12] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-08-14 16:34 UTC (permalink / raw)
To: Wu Hao, Greg KH
Cc: mdf, linux-fpga, linux-kernel, linux-api, linux-doc, atull,
Ananda Ravuri, Xu Yilun
In-Reply-To: <20190724142235.GE8463@hao-dev>
On Wed, 2019-07-24 at 22:22 +0800, Wu Hao wrote:
> On Wed, Jul 24, 2019 at 11:35:32AM +0200, Greg KH wrote:
> > On Tue, Jul 23, 2019 at 12:51:24PM +0800, Wu Hao wrote:
> > >
> > > @@ -67,8 +69,43 @@
> > > #define PR_WAIT_TIMEOUT 8000000
> > > #define PR_HOST_STATUS_IDLE 0
> > >
> > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > +
> > > +#include <linux/cpufeature.h>
> > > +#include <asm/fpu/api.h>
> > > +
> > > +static inline int is_cpu_avx512_enabled(void)
> > > +{
> > > + return cpu_feature_enabled(X86_FEATURE_AVX512F);
> > > +}
> >
> > That's a very arch specific function, why would a driver ever care about
> > this?
>
> Yes, this is only applied to a specific FPGA solution, which FPGA
> has been integrated with XEON. Hardware indicates this using register
> to software. As it's cpu integrated solution, so CPU always has this
> AVX512 capability. The only check we do, is make sure this is not
> manually disabled by kernel.
>
> With this hardware, software could use AVX512 to accelerate the FPGA
> partial reconfiguration as mentioned in the patch commit message.
> It brings performance benifits to people who uses it. This is only one
> optimization (512 vs 32bit data write to hw) for a specific hardware.
I thought earlier you said that 512 bit accesses were required for this
particular integrated-only version of the device, and not just an
optimization?
> > > +#else
> > > +static inline int is_cpu_avx512_enabled(void)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static inline void copy512(const void *src, void __iomem *dst)
> > > +{
> > > + WARN_ON_ONCE(1);
> >
> > Are you trying to get reports from syzbot? :)
>
> Oh.. no.. I will remove it. :)
>
> Thank you very much!
What's wrong with this? The driver should never call copy512() if
is_cpu_avx512_enabled() returns 0, and if syzbot can somehow make the driver
do so, then yes we do want a report.
-Scott
^ permalink raw reply
* Re: [PATCH v5 2/6] mm/page_idle: Add support for handling swapped PG_Idle pages
From: Joel Fernandes @ 2019-08-14 16:32 UTC (permalink / raw)
To: Michal Hocko
Cc: khlebnikov, linux-kernel, Minchan Kim, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Robin Murphy, Roman Gushchin, Stephen Rothwell, surenb,
Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190814080531.GP17933@dhcp22.suse.cz>
On Wed, Aug 14, 2019 at 10:05:31AM +0200, Michal Hocko wrote:
> On Tue 13-08-19 11:36:59, Joel Fernandes wrote:
> > On Tue, Aug 13, 2019 at 05:04:50PM +0200, Michal Hocko wrote:
> > > On Wed 07-08-19 13:15:55, Joel Fernandes (Google) wrote:
> > > > Idle page tracking currently does not work well in the following
> > > > scenario:
> > > > 1. mark page-A idle which was present at that time.
> > > > 2. run workload
> > > > 3. page-A is not touched by workload
> > > > 4. *sudden* memory pressure happen so finally page A is finally swapped out
> > > > 5. now see the page A - it appears as if it was accessed (pte unmapped
> > > > so idle bit not set in output) - but it's incorrect.
> > > >
> > > > To fix this, we store the idle information into a new idle bit of the
> > > > swap PTE during swapping of anonymous pages.
> > > >
> > > > Also in the future, madvise extensions will allow a system process
> > > > manager (like Android's ActivityManager) to swap pages out of a process
> > > > that it knows will be cold. To an external process like a heap profiler
> > > > that is doing idle tracking on another process, this procedure will
> > > > interfere with the idle page tracking similar to the above steps.
> > >
> > > This could be solved by checking the !present/swapped out pages
> > > right? Whoever decided to put the page out to the swap just made it
> > > idle effectively. So the monitor can make some educated guess for
> > > tracking. If that is fundamentally not possible then please describe
> > > why.
> >
> > But the monitoring process (profiler) does not have control over the 'whoever
> > made it effectively idle' process.
>
> Why does that matter? Whether it is a global/memcg reclaim or somebody
> calling MADV_PAGEOUT or whatever it is a decision to make the page not
> hot. Sure you could argue that a missing idle bit on swap entries might
> mean that the swap out decision was pre-mature/sub-optimal/wrong but is
> this the aim of the interface?
>
> > As you said it will be a guess, it will not be accurate.
>
> Yes and the point I am trying to make is that having some space and not
> giving a guarantee sounds like a safer option for this interface because
I do see your point of view, but jJust because a future (and possibly not
going to happen) usecase which you mentioned as pte reclaim, makes you feel
that userspace may be subject to inaccuracies anyway, doesn't mean we should
make everything inaccurate.. We already know idle page tracking is not
completely accurate. But that doesn't mean we miss out on the opportunity to
make the "non pte-reclaim" usecase inaccurate as well.
IMO, we should do our best for today, and not hypothesize. How likely is pte
reclaim and is there a thread to describe that direction?
> > I am curious what is your concern with using a bit in the swap PTE?
>
> ... It is a promiss of the semantic I find limiting for future. The bit
> in the pte might turn out insufficient (e.g. pte reclaim) so teaching
> the userspace to consider this a hard guarantee is a ticket to problems
> later on. Maybe I am overly paranoid because I have seen so many "nice
> to have" features turning into a maintenance burden in the past.
>
> If this is really considered mostly debugging purpouse interface then a
> certain level of imprecision should be tolerateable. If there is a
> really strong real world usecase that simply has no other way to go
> then this might be added later. Adding an information is always safer
> than take it away.
>
> That being said, if I am a minority voice here then I will not really
> stand in the way and won't nack the patch. I will not ack it neither
> though.
Ok.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v8 09/27] mm/mmap: Prevent Shadow Stack VMA merges
From: Yu-cheng Yu @ 2019-08-14 16:20 UTC (permalink / raw)
To: Dave Hansen, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra, Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue,
Dave Martin
In-Reply-To: <5ba3d1b3-5587-e7dd-b9de-9a954172d31f@intel.com>
On Tue, 2019-08-13 at 15:34 -0700, Dave Hansen wrote:
> On 8/13/19 1:52 PM, Yu-cheng Yu wrote:
> > To prevent function call/return spills into the next shadow stack
> > area, do not merge shadow stack areas.
>
> How does this prevent call/return spills?
It does not. I will fix the description.
Yu-cheng
^ permalink raw reply
* [PATCH 2/3] kbuild: rebuild modules when module linker scripts are updated
From: Masahiro Yamada @ 2019-08-14 16:06 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Albert Ou, Benjamin Herrenschmidt,
Catalin Marinas, Fenghua Yu, Geert Uytterhoeven, Helge Deller,
James E.J. Bottomley, Jonathan Corbet, Michael Ellerman,
Michal Marek, Palmer Dabbelt, Paul Mackerras, Paul Walmsley,
Russell King, Tony Luck, Will Deacon, linux-arm-kernel, linux-doc,
linux-ia64, linux-kernel, linux-m68k, linux-parisc, linux-riscv,
linuxppc-dev
In-Reply-To: <20190814160623.24802-1-yamada.masahiro@socionext.com>
Currently, the timestamp of module linker scripts are not checked.
Add them to the dependency of modules so they are correctly rebuilt.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Documentation/kbuild/makefiles.rst | 5 +++++
Makefile | 3 ++-
arch/arm/Makefile | 2 +-
arch/arm64/Makefile | 2 +-
arch/ia64/Makefile | 2 +-
arch/m68k/Makefile | 2 +-
arch/parisc/Makefile | 2 +-
arch/powerpc/Makefile | 2 +-
arch/riscv/Makefile | 2 +-
scripts/Makefile.modpost | 5 +++--
10 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst
index d3448d2c8017..36ba92e199d2 100644
--- a/Documentation/kbuild/makefiles.rst
+++ b/Documentation/kbuild/makefiles.rst
@@ -999,6 +999,11 @@ When kbuild executes, the following steps are followed (roughly):
The linker script with full path. Assigned by the top-level Makefile.
+ KBUILD_LDS_MODULE
+
+ The module linker script with full path. Assigned by the top-level
+ Makefile and additionally by the arch Makefile.
+
KBUILD_VMLINUX_OBJS
All object files for vmlinux. They are linked to vmlinux in the same
diff --git a/Makefile b/Makefile
index 164ca615e2f6..af808837a1f2 100644
--- a/Makefile
+++ b/Makefile
@@ -485,7 +485,8 @@ KBUILD_AFLAGS_KERNEL :=
KBUILD_CFLAGS_KERNEL :=
KBUILD_AFLAGS_MODULE := -DMODULE
KBUILD_CFLAGS_MODULE := -DMODULE
-KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds
+KBUILD_LDFLAGS_MODULE :=
+export KBUILD_LDS_MODULE := $(srctree)/scripts/module-common.lds
KBUILD_LDFLAGS :=
GCC_PLUGINS_CFLAGS :=
CLANG_FLAGS :=
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index c3624ca6c0bc..fbe50eec8f34 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -17,7 +17,7 @@ KBUILD_LDFLAGS_MODULE += --be8
endif
ifeq ($(CONFIG_ARM_MODULE_PLTS),y)
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm/kernel/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/arm/kernel/module.lds
endif
GZFLAGS :=-9
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 61de992bbea3..d4ed1869e536 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -101,7 +101,7 @@ endif
CHECKFLAGS += -D__aarch64__
ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/arm64/kernel/module.lds
endif
# Default value
diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
index 171290f9f1de..5c3bcaee5980 100644
--- a/arch/ia64/Makefile
+++ b/arch/ia64/Makefile
@@ -20,7 +20,7 @@ CHECKFLAGS += -D__ia64=1 -D__ia64__=1 -D_LP64 -D__LP64__
OBJCOPYFLAGS := --strip-all
LDFLAGS_vmlinux := -static
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/ia64/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/ia64/module.lds
KBUILD_AFLAGS_KERNEL := -mconstant-gp
EXTRA :=
diff --git a/arch/m68k/Makefile b/arch/m68k/Makefile
index 482513b9af2c..5d9288384096 100644
--- a/arch/m68k/Makefile
+++ b/arch/m68k/Makefile
@@ -73,7 +73,7 @@ KBUILD_AFLAGS += -D__uClinux__
endif
KBUILD_LDFLAGS := -m m68kelf
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/m68k/kernel/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/m68k/kernel/module.lds
ifdef CONFIG_SUN3
LDFLAGS_vmlinux = -N
diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
index 3b77d729057f..36b834f1c933 100644
--- a/arch/parisc/Makefile
+++ b/arch/parisc/Makefile
@@ -60,7 +60,7 @@ KBUILD_CFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY=1 \
-DFTRACE_PATCHABLE_FUNCTION_SIZE=$(NOP_COUNT)
CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1)))
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/parisc/kernel/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/parisc/kernel/module.lds
endif
OBJCOPY_FLAGS =-O binary -R .note -R .comment -S
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index c345b79414a9..b2227855de20 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -67,7 +67,7 @@ UTS_MACHINE := $(subst $(space),,$(machine-y))
ifdef CONFIG_PPC32
KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
else
-KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/powerpc/kernel/module.lds
+KBUILD_LDS_MODULE += $(srctree)/arch/powerpc/kernel/module.lds
ifeq ($(call ld-ifversion, -ge, 225000000, y),y)
# Have the linker provide sfpr if possible.
# There is a corresponding test in arch/powerpc/lib/Makefile
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 7a117be8297c..426d989125a8 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -52,7 +52,7 @@ ifeq ($(CONFIG_CMODEL_MEDANY),y)
KBUILD_CFLAGS += -mcmodel=medany
endif
ifeq ($(CONFIG_MODULE_SECTIONS),y)
- KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/riscv/kernel/module.lds
+ KBUILD_LDS_MODULE += $(srctree)/arch/riscv/kernel/module.lds
endif
KBUILD_CFLAGS_MODULE += $(call cc-option,-mno-relax)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index bf15818f6947..905db30d6622 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -126,10 +126,11 @@ quiet_cmd_ld_ko_o = LD [M] $@
cmd_ld_ko_o = \
$(LD) -r $(KBUILD_LDFLAGS) \
$(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE) \
- -o $@ $(real-prereqs) ; \
+ $(addprefix -T , $(KBUILD_LDS_MODULE)) \
+ -o $@ $(filter %.o, $^); \
$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
-$(modules): %.ko :%.o %.mod.o FORCE
+$(modules): %.ko :%.o %.mod.o $(KBUILD_LDS_MODULE) FORCE
+$(call if_changed,ld_ko_o)
targets += $(modules)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v8 01/27] Documentation/x86: Add CET description
From: Yu-cheng Yu @ 2019-08-14 15:57 UTC (permalink / raw)
To: Florian Weimer
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <87tvakgofi.fsf@oldenburg2.str.redhat.com>
On Wed, 2019-08-14 at 10:07 +0200, Florian Weimer wrote:
> * Yu-cheng Yu:
>
> > +ENDBR
> > + The compiler inserts an ENDBR at all valid branch targets. Any
> > + CALL/JMP to a target without an ENDBR triggers a control
> > + protection fault.
>
> Is this really correct? I think ENDBR is needed only for indirect
> branch targets where the jump/call does not have a NOTRACK prefix.
You are right. I will fix the wording.
Yu-cheng
^ permalink raw reply
* [PATCH 1/3] kbuild: move KBUILD_LDS, KBUILD_VMLINUX_{OBJS,LIBS} to makefiles.rst
From: Masahiro Yamada @ 2019-08-14 16:06 UTC (permalink / raw)
To: linux-kbuild
Cc: Masahiro Yamada, Jonathan Corbet, Michal Marek, linux-doc,
linux-kernel
These three variables are not intended to be tweaked by users.
Move them from kbuild.rst to makefiles.rst.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
I will apply to linux-kbuild this
to avoid conflicts.
Documentation/kbuild/kbuild.rst | 14 --------------
Documentation/kbuild/makefiles.rst | 14 ++++++++++++++
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
index 61b2181ed3ea..62f9d86c082c 100644
--- a/Documentation/kbuild/kbuild.rst
+++ b/Documentation/kbuild/kbuild.rst
@@ -258,17 +258,3 @@ KBUILD_BUILD_USER, KBUILD_BUILD_HOST
These two variables allow to override the user@host string displayed during
boot and in /proc/version. The default value is the output of the commands
whoami and host, respectively.
-
-KBUILD_LDS
-----------
-The linker script with full path. Assigned by the top-level Makefile.
-
-KBUILD_VMLINUX_OBJS
--------------------
-All object files for vmlinux. They are linked to vmlinux in the same
-order as listed in KBUILD_VMLINUX_OBJS.
-
-KBUILD_VMLINUX_LIBS
--------------------
-All .a "lib" files for vmlinux. KBUILD_VMLINUX_OBJS and KBUILD_VMLINUX_LIBS
-together specify all the object files used to link vmlinux.
diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst
index f4f0f7ffde2b..d3448d2c8017 100644
--- a/Documentation/kbuild/makefiles.rst
+++ b/Documentation/kbuild/makefiles.rst
@@ -995,6 +995,20 @@ When kbuild executes, the following steps are followed (roughly):
top-level Makefile has set any other flags. This provides a
means for an architecture to override the defaults.
+ KBUILD_LDS
+
+ The linker script with full path. Assigned by the top-level Makefile.
+
+ KBUILD_VMLINUX_OBJS
+
+ All object files for vmlinux. They are linked to vmlinux in the same
+ order as listed in KBUILD_VMLINUX_OBJS.
+
+ KBUILD_VMLINUX_LIBS
+
+ All .a "lib" files for vmlinux. KBUILD_VMLINUX_OBJS and
+ KBUILD_VMLINUX_LIBS together specify all the object files used to
+ link vmlinux.
6.2 Add prerequisites to archheaders
------------------------------------
--
2.17.1
^ permalink raw reply related
* Re: [UNVERIFIED SENDER] Re: [PATCH 0/9] arm64: Stolen time support
From: Alexander Graf @ 2019-08-14 14:52 UTC (permalink / raw)
To: Marc Zyngier
Cc: Steven Price, kvm, Catalin Marinas, linux-doc, Russell King,
linux-kernel, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <8636i3omnd.wl-maz@kernel.org>
On 14.08.19 16:19, Marc Zyngier wrote:
> On Wed, 14 Aug 2019 14:02:25 +0100,
> Alexander Graf <graf@amazon.com> wrote:
>>
>>
>>
>> On 05.08.19 15:06, Steven Price wrote:
>>> On 03/08/2019 19:05, Marc Zyngier wrote:
>>>> On Fri, 2 Aug 2019 15:50:08 +0100
>>>> Steven Price <steven.price@arm.com> wrote:
>>>>
>>>> Hi Steven,
>>>>
>>>>> This series add support for paravirtualized time for arm64 guests and
>>>>> KVM hosts following the specification in Arm's document DEN 0057A:
>>>>>
>>>>> https://developer.arm.com/docs/den0057/a
>>>>>
>>>>> It implements support for stolen time, allowing the guest to
>>>>> identify time when it is forcibly not executing.
>>>>>
>>>>> It doesn't implement support for Live Physical Time (LPT) as there are
>>>>> some concerns about the overheads and approach in the above
>>>>> specification, and I expect an updated version of the specification to
>>>>> be released soon with just the stolen time parts.
>>>>
>>>> Thanks for posting this.
>>>>
>>>> My current concern with this series is around the fact that we allocate
>>>> memory from the kernel on behalf of the guest. It is the first example
>>>> of such thing in the ARM port, and I can't really say I'm fond of it.
>>>>
>>>> x86 seems to get away with it by having the memory allocated from
>>>> userspace, why I tend to like more. Yes, put_user is more
>>>> expensive than a straight store, but this isn't done too often either.
>>>>
>>>> What is the rational for your current approach?
>>>
>>> As I see it there are 3 approaches that can be taken here:
>>>
>>> 1. Hypervisor allocates memory and adds it to the virtual machine. This
>>> means that everything to do with the 'device' is encapsulated behind the
>>> KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want the
>>> stolen time structure to be fast it cannot be a trapping region and has
>>> to be backed by real memory - in this case allocated by the host kernel.
>>>
>>> 2. Host user space allocates memory. Similar to above, but this time
>>> user space needs to manage the memory region as well as the usual
>>> KVM_CREATE_DEVICE dance. I've no objection to this, but it means
>>> kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
>>> to size the memory region).
>>
>> You ideally want to get the host overhead for a VM to as little as you
>> can. I'm not terribly fond of the idea of reserving a full page just
>> because we're too afraid of having the guest donate memory.
>
> Well, reduce the amount of memory you give to the guest by one page,
> and allocate that page to the stolen time device. Problem solved!
>
> Seriously, if you're worried about the allocation of a single page,
> you should first look at how many holes we have in the vcpu structure,
> for example (even better, with the 8.4 NV patches applied). Just
> fixing that would give you that page back *per vcpu*.
I'm worried about additional memory slots, about fragmenting the
cachable guest memory regions, about avoidable HV taxes.
I think we need to distinguish here between the KVM implementation and
the hypervisor/guest interface. Just because in KVM we can save overhead
today doesn't mean that the HV interface should be built around the
assumption that "memory is free".
>
>>> 3. Guest kernel "donates" the memory to the hypervisor for the
>>> structure. As far as I'm aware this is what x86 does. The problems I see
>>> this approach are:
>>>
>>> a) kexec becomes much more tricky - there needs to be a disabling
>>> mechanism for the guest to stop the hypervisor scribbling on memory
>>> before starting the new kernel.
>>
>> I wouldn't call "quiesce a device" much more tricky. We have to do
>> that for other devices as well today.
>
> And since there is no standard way of doing it, we keep inventing
> weird and wonderful ways of doing so -- cue the terrible GICv3 LPI
> situation, and all the various hacks to keep existing IOMMU mappings
> around across firmware/kernel handovers as well as kexec.
Well, the good news here is that we don't have to keep it around ;).
>
>>
>>> b) If there is more than one entity that is interested in the
>>> information (e.g. firmware and kernel) then this requires some form of
>>> arbitration in the guest because the hypervisor doesn't want to have to
>>> track an arbitrary number of regions to update.
>>
>> Why would FW care?
>
> Exactly. It doesn't care. Not caring means it doesn't know about the
> page the guest has allocated for stolen time, and starts using it for
> its own purposes. Hello, memory corruption. Same thing goes if you
> reboot into a non stolen time aware kernel.
If you reboot, you go via the vcpu reset path which clears the map, no?
Same goes for FW entry. If you enter firmware that does not set up the
map, you never see it.
>
>>
>>> c) Performance can suffer if the host kernel doesn't have a suitably
>>> aligned/sized area to use. As you say - put_user() is more expensive.
>>
>> Just define the interface to always require natural alignment when
>> donating a memory location?
>>
>>> The structure is updated on every return to the VM.
>>
>> If you really do suffer from put_user(), there are alternatives. You
>> could just map the page on the registration hcall and then leave it
>> pinned until the vcpu gets destroyed again.
>
> put_user() should be cheap enough. It is one of the things we tend to
> optimise anyway. And yes, worse case, we pin the page.
>
>>
>>> Of course x86 does prove the third approach can work, but I'm not sure
>>> which is actually better. Avoid the kexec cancellation requirements was
>>> the main driver of the current approach. Although many of the
>>
>> I really don't understand the problem with kexec cancellation. Worst
>> case, let guest FW set it up for you and propagate only the address
>> down via ACPI/DT. That way you can mark the respective memory as
>> reserved too.
>
> We already went down that road with the LPI hack. I'm not going there
> again if we can avoid it. And it turn out that we can. Just allocate
> the stolen time page as a separate memblock, give it to KVM for that
> purpose.
>
> Your suggestion of letting the guest firmware set something up only
> works if whatever you're booting after that understands it. If it
> doesn't, you're screwed.
Why? For UEFI, mark the region as reserved in the memory map. For DT,
just mark it straight on reserved.
That said, I'm not advocating for doing it in the FW. I think this can
be solved really easily with a simple guest driver to enable and a vcpu
reset hook to disable the map.
>
>> But even with a Linux only mechanism, just take a look at
>> arch/x86/kernel/kvmclock.c. All they do to remove the map is to hook
>> into machine_crash_shutdown() and machine_shutdown().
>
> I'm not going to take something that is Linux specific. It has to work
> for all guests, at all times, whether they know about the hypervisor
> service or not.
If they don't know about the HV service, they don't register the writer,
so they don't see corruption.
If they know about the HV service and they don't support kexec, they
don't have to worry because a vcpu reset should also clear the map.
If they do support kexec, they already have a mechanism to quiesce devices.
So I don't understand how this is Linux specific? The question was Linux
specific, so I answered with precedence to show that disabling on kexec
is not all that hard :).
Alex
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Joel Fernandes @ 2019-08-14 14:38 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190813190738.GH28441@linux.ibm.com>
On Tue, Aug 13, 2019 at 12:07:38PM -0700, Paul E. McKenney wrote:
[snip]
> > This patch adds basic batching support for kfree_rcu(). It is "basic"
> > because we do none of the slab management, dynamic allocation, code
> > moving or any of the other things, some of which previous attempts did
> > [2]. These fancier improvements can be follow-up patches and there are
> > different ideas being discussed in those regards.
> >
> > Torture tests follow in the next patch and show improvements of around
> > 400% in reduction of number of grace periods on a 16 CPU system. More
>
> s/400% in reduction/a 5x reduction/
Ok. That's more clear.
> > details and test data are in that patch.
> >
> > This is an effort to start simple, and build up from there. In the
> > future, an extension to use kfree_bulk and possibly per-slab batching
> > could be done to further improve performance due to cache-locality and
> > slab-specific bulk free optimizations. By using an array of pointers,
> > the worker thread processing the work would need to read lesser data
> > since it does not need to deal with large rcu_head(s) any longer.
>
> This should be combined with the second paragraph -- they both discuss
> possible follow-on work.
Ack.
> > There is an implication with rcu_barrier() with this patch. Since the
> > kfree_rcu() calls can be batched, and may not be handed yet to the RCU
> > machinery in fact, the monitor may not have even run yet to do the
> > queue_rcu_work(), there seems no easy way of implementing rcu_barrier()
> > to wait for those kfree_rcu()s that are already made. So this means a
> > kfree_rcu() followed by an rcu_barrier() does not imply that memory will
> > be freed once rcu_barrier() returns.
>
> The usual approach (should kfree_rcu_barrier() in fact be needed) is to
> record the address of the most recent block being kfree_rcu()'d on
> each CPU, count the number of recorded addresses, and have the
> kfree() side check the address, and do the usual atomic_dec_and_test()
> and wakeup dance. This gets a bit more crazy should the kfree_rcu()
> requests be grouped per slab, of course! So yes, good to avoid in
> the meantime.
Good idea!
> > Another implication is higher active memory usage (although not
> > run-away..) until the kfree_rcu() flooding ends, in comparison to
> > without batching. More details about this are in the second patch which
> > adds an rcuperf test.
>
> But this is adjustable, at least via patching at build time, via
> KFREE_DRAIN_JIFFIES, right?
Yes.
> > Finally, in the near future we will get rid of kfree_rcu special casing
> > within RCU such as in rcu_do_batch and switch everything to just
> > batching. Currently we don't do that since timer subsystem is not yet up
> > and we cannot schedule the kfree_rcu() monitor as the timer subsystem's
> > lock are not initialized. That would also mean getting rid of
> > kfree_call_rcu_nobatch() entirely.
>
> Please consistently put "()" after function names.
Done.
> > [1] http://lore.kernel.org/lkml/20190723035725-mutt-send-email-mst@kernel.org
> > [2] https://lkml.org/lkml/2017/12/19/824
> >
> > Cc: Rao Shoaib <rao.shoaib@oracle.com>
> > Cc: max.byungchul.park@gmail.com
> > Cc: byungchul.park@lge.com
> > Cc: kernel-team@android.com
> > Cc: kernel-team@lge.com
> > Co-developed-by: Byungchul Park <byungchul.park@lge.com>
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > ---
> > v2->v3: Just some code comment changes thanks to Byungchul.
> >
> > RFCv1->PATCH v2: Removed limits on the ->head list, just let it grow.
> > Dropped KFREE_MAX_JIFFIES to HZ/50 from HZ/20 to reduce OOM occurrence.
> > Removed sleeps in rcuperf test, just using cond_resched()in loop.
> > Better code comments ;)
> >
> > .../admin-guide/kernel-parameters.txt | 17 ++
> > include/linux/rcutiny.h | 5 +
> > include/linux/rcutree.h | 1 +
> > kernel/rcu/tree.c | 204 +++++++++++++++++-
> > 4 files changed, 221 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 7ccd158b3894..a9156ca5de24 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3895,6 +3895,23 @@
> > test until boot completes in order to avoid
> > interference.
> >
> > + rcuperf.kfree_rcu_test= [KNL]
> > + Set to measure performance of kfree_rcu() flooding.
> > +
> > + rcuperf.kfree_nthreads= [KNL]
> > + The number of threads running loops of kfree_rcu().
> > +
> > + rcuperf.kfree_alloc_num= [KNL]
> > + Number of allocations and frees done in an iteration.
> > +
> > + rcuperf.kfree_loops= [KNL]
> > + Number of loops doing rcuperf.kfree_alloc_num number
> > + of allocations and frees.
> > +
> > + rcuperf.kfree_no_batch= [KNL]
> > + Use the non-batching (slower) version of kfree_rcu.
> > + This is useful for comparing with the batched version.
> > +
> > rcuperf.nreaders= [KNL]
> > Set number of RCU readers. The value -1 selects
> > N, where N is the number of CPUs. A value
>
> This change to kernel-parameters.txt really needs to be in the
> rcuperf patch rather than this one.
Fixed.
FWIW, my new tool (sort of under wraps) to move hunks from one patch to
another in a git tree makes this easy: https://github.com/joelagnel/git-edit
(I hit 'p' to edit patches and it commits back the results when I close the
editor).
[snip]
> > - * Queue an RCU callback for lazy invocation after a grace period.
> > - * This will likely be later named something like "call_rcu_lazy()",
> > - * but this change will require some way of tagging the lazy RCU
> > - * callbacks in the list of pending callbacks. Until then, this
> > - * function may only be called from __kfree_rcu().
> > + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> > + * kfree(s) is queued for freeing after a grace period, right away.
> > */
> > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > +struct kfree_rcu_cpu {
> > + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> > + * is done after a grace period.
> > + */
> > + struct rcu_work rcu_work;
> > +
> > + /* The list of objects being queued in a batch but are not yet
> > + * scheduled to be freed.
> > + */
> > + struct rcu_head *head;
> > +
> > + /* The list of objects that have now left ->head and are queued for
> > + * freeing after a grace period.
> > + */
> > + struct rcu_head *head_free;
>
> So this is not yet the one that does multiple batches concurrently
> awaiting grace periods, correct? Or am I missing something subtle?
Yes, it is not. I honestly, still did not understand that idea. Or how it
would improve things. May be we can discuss at LPC on pen and paper? But I
think that can also be a follow-up optimization.
> If it is not doing the multiple-batch optimization, what does the
> 400% in the commit log refer to?
The 400% is the reduction in the number of grace periods just with this patch
(single batch processed at a time). The improvement is because of batching, we end
up having to start fewer grace periods while still managing to attend to the
kfree_rcu() load.
There is concurrency, because ->head continues to grow while ->head_free is
being processed.
> > + /* Protect concurrent access to this structure. */
> > + spinlock_t lock;
> > +
> > + /* The delayed work that flushes ->head to ->head_free incase ->head
> > + * within KFREE_DRAIN_JIFFIES. In case flushing cannot be done if RCU
> > + * is busy, ->head just continues to grow and we retry flushing later.
> > + */
> > + struct delayed_work monitor_work;
> > + bool monitor_todo; /* Is a delayed work pending execution? */
> > +};
> > +
> > +static DEFINE_PER_CPU(struct kfree_rcu_cpu, krc);
> > +
> > +/*
> > + * This function is invoked in workqueue context after a grace period.
> > + * It frees all the objects queued on ->head_free.
> > + */
> > +static void kfree_rcu_work(struct work_struct *work)
> > +{
> > + unsigned long flags;
> > + struct rcu_head *head, *next;
> > + struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
> > + struct kfree_rcu_cpu, rcu_work);
> > +
> > + spin_lock_irqsave(&krcp->lock, flags);
> > + head = krcp->head_free;
> > + krcp->head_free = NULL;
> > + spin_unlock_irqrestore(&krcp->lock, flags);
> > +
> > + /*
> > + * The head is detached and not referenced from anywhere, so lockless
> > + * access is Ok.
> > + */
> > + for (; head; head = next) {
> > + next = head->next;
> > + head->next = NULL;
>
> Why do we need to NULL ->next? Could produce an expensive cache miss,
> and I don't see how it helps anything. What am I missing here?
You're right. I will get rid of hit, no reason to have it.
[snip]
> > +static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> > + unsigned long flags)
> > +{
> > + /* Flush ->head to ->head_free, all objects on ->head_free will be
> > + * kfree'd after a grace period.
> > + */
> > + if (queue_kfree_rcu_work(krcp)) {
> > + /* Success! Our job is done here. */
> > + spin_unlock_irqrestore(&krcp->lock, flags);
> > + return;
> > + }
> > +
> > + /* Previous batch did not get free yet, let us try again soon. */
> > + if (krcp->monitor_todo == false) {
> > + schedule_delayed_work_on(smp_processor_id(),
> > + &krcp->monitor_work, KFREE_DRAIN_JIFFIES);
>
> Given that we are using a lock, do we need to restrict to the current
> CPU? In any case, we do need to handle the case where the current
> CPU has gone offline before we get around to sending a batch off to
> wait for a grace period, correct?
The results either way are similar, but I agree it is better to not use the
CPU specific API, and use the generic one especially because it tries to
queue on the local CPU if possible.
Will respin!
thanks,
- Joel
^ permalink raw reply
* [PATCH 00/10 v2] Cleanup IOMMU passthrough setting (and disable IOMMU Passthrough when SME is active)
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit
Hi,
This patch-set started out small to overwrite the default passthrough
setting (through CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y) when SME is active.
But on the way to that Tom reminded me that the current ways to
configure passthrough/no-passthrough modes for IOMMU on x86 is a mess.
So I added a few more patches to clean that up a bit, getting rid of the
iommu_pass_through variable on the way.This information is now kept only
in iommu code, with helpers to change that setting from architecture
code.
And of course this patch-set still disables IOMMU Passthrough mode when
SME is active even when CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y is set.
The reason for that change is that SME with passthrough mode turned out
to be fragile with devices requiring SWIOTLB, mainly because SWIOTLB has
a maximum allocation size of 256kb and a limit overall size of the
bounce buffer.
Therefore having IOMMU in translation mode by default is better when SME
is active on a system.
Please review.
Thanks,
Joerg
Changes since v1:
- Cleaned up the kernel command line parameters to
configure passthrough/translated mode, getting rid
of the global iommu_pass_through variable
Joerg Roedel (10):
iommu: Add helpers to set/get default domain type
iommu/amd: Request passthrough mode from IOMMU core
iommu/vt-d: Request passthrough mode from IOMMU core
x86/dma: Get rid of iommu_pass_through
ia64: Get rid of iommu_pass_through
iommu: Remember when default domain type was set on kernel command
line
iommu: Print default domain type on boot
iommu: Set default domain type at runtime
iommu: Disable passthrough mode when SME is active
Documentation: Update Documentation for iommu.passthrough
.../admin-guide/kernel-parameters.txt | 2 +-
arch/ia64/include/asm/iommu.h | 2 -
arch/ia64/kernel/pci-dma.c | 2 -
arch/x86/include/asm/iommu.h | 1 -
arch/x86/kernel/pci-dma.c | 11 +--
drivers/iommu/amd_iommu.c | 6 +-
drivers/iommu/intel-iommu.c | 2 +-
drivers/iommu/iommu.c | 83 +++++++++++++++++--
include/linux/iommu.h | 16 ++++
9 files changed, 101 insertions(+), 24 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 01/10] iommu: Add helpers to set/get default domain type
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Add a couple of functions to allow changing the default
domain type from architecture code and a function for iommu
drivers to request whether the default domain is
passthrough.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/iommu.c | 16 ++++++++++++++++
include/linux/iommu.h | 16 ++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c674d80c37f..f187e85a074b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2196,6 +2196,22 @@ int iommu_request_dma_domain_for_dev(struct device *dev)
return request_default_domain_for_dev(dev, IOMMU_DOMAIN_DMA);
}
+void iommu_set_default_passthrough(void)
+{
+ iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
+}
+
+void iommu_set_default_translated(void)
+{
+ iommu_def_domain_type = IOMMU_DOMAIN_DMA;
+}
+
+bool iommu_default_passthrough(void)
+{
+ return iommu_def_domain_type == IOMMU_DOMAIN_IDENTITY;
+}
+EXPORT_SYMBOL_GPL(iommu_default_passthrough);
+
const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
{
const struct iommu_ops *ops = NULL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fdc355ccc570..58c3e3e5f157 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -413,6 +413,9 @@ extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
extern int iommu_request_dm_for_dev(struct device *dev);
extern int iommu_request_dma_domain_for_dev(struct device *dev);
+extern void iommu_set_default_passthrough(void);
+extern void iommu_set_default_translated(void);
+extern bool iommu_default_passthrough(void);
extern struct iommu_resv_region *
iommu_alloc_resv_region(phys_addr_t start, size_t length, int prot,
enum iommu_resv_type type);
@@ -694,6 +697,19 @@ static inline int iommu_request_dma_domain_for_dev(struct device *dev)
return -ENODEV;
}
+static inline void iommu_set_default_passthrough(void)
+{
+}
+
+static inline void iommu_set_default_translated(void)
+{
+}
+
+static inline bool iommu_default_passthrough(void)
+{
+ return true;
+}
+
static inline int iommu_attach_group(struct iommu_domain *domain,
struct iommu_group *group)
{
--
2.17.1
^ permalink raw reply related
* [PATCH 02/10] iommu/amd: Request passthrough mode from IOMMU core
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Get rid of the iommu_pass_through variable and request
passthrough mode via the new iommu core function.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/amd_iommu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index b607a92791d3..7434b34d7a94 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -436,7 +436,7 @@ static int iommu_init_device(struct device *dev)
* invalid address), we ignore the capability for the device so
* it'll be forced to go into translation mode.
*/
- if ((iommu_pass_through || !amd_iommu_force_isolation) &&
+ if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
dev_is_pci(dev) && pci_iommuv2_capable(to_pci_dev(dev))) {
struct amd_iommu *iommu;
@@ -2226,7 +2226,7 @@ static int amd_iommu_add_device(struct device *dev)
BUG_ON(!dev_data);
- if (iommu_pass_through || dev_data->iommu_v2)
+ if (dev_data->iommu_v2)
iommu_request_dm_for_dev(dev);
/* Domains are initialized for this device - have a look what we ended up with */
@@ -2805,7 +2805,7 @@ int __init amd_iommu_init_api(void)
int __init amd_iommu_init_dma_ops(void)
{
- swiotlb = (iommu_pass_through || sme_me_mask) ? 1 : 0;
+ swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
iommu_detected = 1;
if (amd_iommu_unmap_flush)
--
2.17.1
^ permalink raw reply related
* [PATCH 05/10] ia64: Get rid of iommu_pass_through
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
This variable has no users anymore so it can be removed.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
arch/ia64/include/asm/iommu.h | 2 --
arch/ia64/kernel/pci-dma.c | 2 --
2 files changed, 4 deletions(-)
diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
index 7429a72f3f92..92aceef63710 100644
--- a/arch/ia64/include/asm/iommu.h
+++ b/arch/ia64/include/asm/iommu.h
@@ -8,10 +8,8 @@
extern void no_iommu_init(void);
#ifdef CONFIG_INTEL_IOMMU
extern int force_iommu, no_iommu;
-extern int iommu_pass_through;
extern int iommu_detected;
#else
-#define iommu_pass_through (0)
#define no_iommu (1)
#define iommu_detected (0)
#endif
diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index fe988c49f01c..f5d49cd3fbb0 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -22,8 +22,6 @@ int force_iommu __read_mostly = 1;
int force_iommu __read_mostly;
#endif
-int iommu_pass_through;
-
static int __init pci_iommu_init(void)
{
if (iommu_detected)
--
2.17.1
^ permalink raw reply related
* [PATCH 09/10] iommu: Disable passthrough mode when SME is active
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Using Passthrough mode when SME is active causes certain
devices to use the SWIOTLB bounce buffer. The bounce buffer
code has an upper limit of 256kb for the size of DMA
allocations, which is too small for certain devices and
causes them to fail.
With this patch we enable IOMMU by default when SME is
active in the system, making the default configuration work
for more systems than it does now.
Users that don't want IOMMUs to be enabled still can disable
them with kernel parameters.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/iommu.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 96cc7cc8ab21..4bc9025a9975 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -119,6 +119,11 @@ static int __init iommu_subsys_init(void)
iommu_set_default_passthrough();
else
iommu_set_default_translated();
+
+ if (iommu_default_passthrough() && sme_active()) {
+ pr_info("SME detected - Disabling default IOMMU Passthrough\n");
+ iommu_set_default_translated();
+ }
}
pr_info("Default domain type: %s %s\n",
--
2.17.1
^ permalink raw reply related
* [PATCH 04/10] x86/dma: Get rid of iommu_pass_through
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
This variable has no users anymore. Remove it and tell the
IOMMU code via its new functions about requested DMA modes.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
arch/x86/include/asm/iommu.h | 1 -
arch/x86/kernel/pci-dma.c | 11 +++--------
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index baedab8ac538..b91623d521d9 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -4,7 +4,6 @@
extern int force_iommu, no_iommu;
extern int iommu_detected;
-extern int iommu_pass_through;
/* 10 seconds */
#define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index f62b498b18fb..a6fd479d4a71 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/dma-direct.h>
#include <linux/dma-debug.h>
+#include <linux/iommu.h>
#include <linux/dmar.h>
#include <linux/export.h>
#include <linux/memblock.h>
@@ -43,12 +44,6 @@ int iommu_detected __read_mostly = 0;
* It is also possible to disable by default in kernel config, and enable with
* iommu=nopt at boot time.
*/
-#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
-int iommu_pass_through __read_mostly = 1;
-#else
-int iommu_pass_through __read_mostly;
-#endif
-
extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
void __init pci_iommu_alloc(void)
@@ -120,9 +115,9 @@ static __init int iommu_setup(char *p)
swiotlb = 1;
#endif
if (!strncmp(p, "pt", 2))
- iommu_pass_through = 1;
+ iommu_set_default_passthrough();
if (!strncmp(p, "nopt", 4))
- iommu_pass_through = 0;
+ iommu_set_default_translated();
gart_parse_options(p);
--
2.17.1
^ permalink raw reply related
* [PATCH 10/10] Documentation: Update Documentation for iommu.passthrough
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
This kernel parameter now takes also effect on X86.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
Documentation/admin-guide/kernel-parameters.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 47d981a86e2f..2d5dfa46e88a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1811,7 +1811,7 @@
synchronously.
iommu.passthrough=
- [ARM64] Configure DMA to bypass the IOMMU by default.
+ [ARM64, X86] Configure DMA to bypass the IOMMU by default.
Format: { "0" | "1" }
0 - Use IOMMU translation for DMA.
1 - Bypass the IOMMU for DMA.
--
2.17.1
^ permalink raw reply related
* [PATCH 07/10] iommu: Print default domain type on boot
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Introduce a subsys_initcall for IOMMU code and use it to
print the default domain type at boot.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/iommu.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e1feb4061b8b..233bc22b487e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -93,12 +93,40 @@ struct iommu_group_attribute iommu_group_attr_##_name = \
static LIST_HEAD(iommu_device_list);
static DEFINE_SPINLOCK(iommu_device_lock);
+/*
+ * Use a function instead of an array here because the domain-type is a
+ * bit-field, so an array would waste memory.
+ */
+static const char *iommu_domain_type_str(unsigned int t)
+{
+ switch (t) {
+ case IOMMU_DOMAIN_BLOCKED:
+ return "Blocked";
+ case IOMMU_DOMAIN_IDENTITY:
+ return "Passthrough";
+ case IOMMU_DOMAIN_UNMANAGED:
+ return "Unmanaged";
+ case IOMMU_DOMAIN_DMA:
+ return "Translated";
+ default:
+ return "Unknown";
+ }
+}
+
+static int __init iommu_subsys_init(void)
+{
+ pr_info("Default domain type: %s\n",
+ iommu_domain_type_str(iommu_def_domain_type));
+
+ return 0;
+}
+subsys_initcall(iommu_subsys_init);
+
int iommu_device_register(struct iommu_device *iommu)
{
spin_lock(&iommu_device_lock);
list_add_tail(&iommu->list, &iommu_device_list);
spin_unlock(&iommu_device_lock);
-
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 03/10] iommu/vt-d: Request passthrough mode from IOMMU core
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Get rid of the iommu_pass_through variable and request
passthrough mode via the new iommu core function.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/intel-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index bdaed2da8a55..234bc2b55c59 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3267,7 +3267,7 @@ static int __init init_dmars(void)
iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
}
- if (iommu_pass_through)
+ if (iommu_default_passthrough())
iommu_identity_mapping |= IDENTMAP_ALL;
#ifdef CONFIG_INTEL_IOMMU_BROKEN_GFX_WA
--
2.17.1
^ permalink raw reply related
* [PATCH 06/10] iommu: Remember when default domain type was set on kernel command line
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Introduce an extensible concept to remember when certain
configuration settings for the IOMMU code have been set on
the kernel command line.
This will be used later to prevent overwriting these
settings with other defaults.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/iommu.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f187e85a074b..e1feb4061b8b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
#endif
static bool iommu_dma_strict __read_mostly = true;
+static u32 iommu_cmd_line __read_mostly;
struct iommu_group {
struct kobject kobj;
@@ -68,6 +69,18 @@ static const char * const iommu_group_resv_type_string[] = {
[IOMMU_RESV_SW_MSI] = "msi",
};
+#define IOMMU_CMD_LINE_DMA_API (1 << 0)
+
+static void iommu_set_cmd_line_dma_api(void)
+{
+ iommu_cmd_line |= IOMMU_CMD_LINE_DMA_API;
+}
+
+static bool __maybe_unused iommu_cmd_line_dma_api(void)
+{
+ return !!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API);
+}
+
#define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
struct iommu_group_attribute iommu_group_attr_##_name = \
__ATTR(_name, _mode, _show, _store)
@@ -165,6 +178,8 @@ static int __init iommu_set_def_domain_type(char *str)
if (ret)
return ret;
+ iommu_set_cmd_line_dma_api();
+
iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 08/10] iommu: Set default domain type at runtime
From: Joerg Roedel @ 2019-08-14 13:38 UTC (permalink / raw)
To: Joerg Roedel
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-1-joro@8bytes.org>
From: Joerg Roedel <jroedel@suse.de>
Set the default domain-type at runtime, not at compile-time.
This keeps default domain type setting in one place when we
have to change it at runtime.
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
drivers/iommu/iommu.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 233bc22b487e..96cc7cc8ab21 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -26,11 +26,8 @@
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
-#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
-static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
-#else
-static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
-#endif
+
+static unsigned int iommu_def_domain_type __read_mostly;
static bool iommu_dma_strict __read_mostly = true;
static u32 iommu_cmd_line __read_mostly;
@@ -76,7 +73,7 @@ static void iommu_set_cmd_line_dma_api(void)
iommu_cmd_line |= IOMMU_CMD_LINE_DMA_API;
}
-static bool __maybe_unused iommu_cmd_line_dma_api(void)
+static bool iommu_cmd_line_dma_api(void)
{
return !!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API);
}
@@ -115,8 +112,18 @@ static const char *iommu_domain_type_str(unsigned int t)
static int __init iommu_subsys_init(void)
{
- pr_info("Default domain type: %s\n",
- iommu_domain_type_str(iommu_def_domain_type));
+ bool cmd_line = iommu_cmd_line_dma_api();
+
+ if (!cmd_line) {
+ if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
+ iommu_set_default_passthrough();
+ else
+ iommu_set_default_translated();
+ }
+
+ pr_info("Default domain type: %s %s\n",
+ iommu_domain_type_str(iommu_def_domain_type),
+ cmd_line ? "(set via kernel command line)" : "");
return 0;
}
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox