* RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Christopher M. Riedl @ 2021-02-01 17:43 UTC (permalink / raw)
To: David Laight, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <f6234b06ccb54cffb3583f40635636d3@AcuMS.aculab.com>
On Mon Feb 1, 2021 at 11:37 AM CST, David Laight wrote:
> From: Christopher M. Riedl
> > Sent: 01 February 2021 16:55
> ...
> > > > > > + int i; \
> > > > > > + \
> > > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
> > > > > > + label); \
> > > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > > > > > + __t->thread.TS_FPR(i) = buf[i]; \
> > > > > > + __t->thread.fp_state.fpscr = buf[i]; \
> > > > > > +} while (0)
> > >
> > > On further reflection, since you immediately loop through the buffer
> > > why not just use user_access_begin() and unsafe_get_user() in the loop.
> >
> > Christophe had suggested this a few revisions ago as well. When I tried
> > this approach, the signal handling performance took a pretty big hit:
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html
> >
> > I included some numbers on v3 as well but decided to drop the approach
> > altogether for this one since it just didn't seem worth the hit.
>
> Was that using unsafe_get_user (which relies on user_access_begin()
> having 'opened up' user accesses) or just get_user() that does
> it for every access?
>
> The former should be ok, the latter will be horrid.
It was using unsafe_get_user() whereas unsafe_copy_from_user() will just
call the optimized __copy_tofrom_user() a single time - assuming that
user access is open.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply
* RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: David Laight @ 2021-02-01 17:37 UTC (permalink / raw)
To: 'Christopher M. Riedl', linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C8YCOH19N9EX.3LXG80WZT1N37@geist>
From: Christopher M. Riedl
> Sent: 01 February 2021 16:55
...
> > > > > + int i; \
> > > > > + \
> > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
> > > > > + label); \
> > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > > > > + __t->thread.TS_FPR(i) = buf[i]; \
> > > > > + __t->thread.fp_state.fpscr = buf[i]; \
> > > > > +} while (0)
> >
> > On further reflection, since you immediately loop through the buffer
> > why not just use user_access_begin() and unsafe_get_user() in the loop.
>
> Christophe had suggested this a few revisions ago as well. When I tried
> this approach, the signal handling performance took a pretty big hit:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html
>
> I included some numbers on v3 as well but decided to drop the approach
> altogether for this one since it just didn't seem worth the hit.
Was that using unsafe_get_user (which relies on user_access_begin()
having 'opened up' user accesses) or just get_user() that does
it for every access?
The former should be ok, the latter will be horrid.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Gabriel Paubert @ 2021-02-01 16:54 UTC (permalink / raw)
To: Christopher M. Riedl; +Cc: David Laight, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C8YBET4IGYGF.3QYANVRRHMV0R@geist>
On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote:
> On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> > From: Christopher M. Riedl
> > > Sent: 28 January 2021 04:04
> > >
> > > Reuse the "safe" implementation from signal.c except for calling
> > > unsafe_copy_from_user() to copy into a local buffer.
> > >
> > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > > ---
> > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > > index 2559a681536e..c18402d625f1 100644
> > > --- a/arch/powerpc/kernel/signal.h
> > > +++ b/arch/powerpc/kernel/signal.h
> > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > &buf[i], label);\
> > > } while (0)
> > >
> > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \
> > > + struct task_struct *__t = task; \
> > > + u64 __user *__f = (u64 __user *)from; \
> > > + u64 buf[ELF_NFPREG]; \
> >
> > How big is that buffer?
> > Isn't is likely to be reasonably large compared to a reasonable
> > kernel stack frame.
> > Especially since this isn't even a leaf function.
> >
>
> I think Christophe answered this - I don't really have an opinion either
> way. What would be a 'reasonable' kernel stack frame for reference?
See include/linux/poll.h, where the limit is of the order of 800 bytes
and the number of entries in an on stack array is chosen at compile time
(different between 32 and 64 bit for example).
The values are used in do_sys_poll, which, with almost 1000 bytes of
stack footprint, appears close to the top of "make checkstack". In
addition do_sys_poll has to call the ->poll function of every file
descriptor in its table, so it is not a tail function.
This 264 bytes array looks reasonable, but please use 'make checkstack'
to verify that the function's total stack usage stays within reason.
Gabriel
>
> > > + int i; \
> > > + \
> > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
> >
> > That really ought to be sizeof(buf).
> >
>
> Agreed, I will fix this. Thanks!
>
> > David
> >
> >
> > > + label); \
> > > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > > + __t->thread.TS_FPR(i) = buf[i]; \
> > > + __t->thread.fp_state.fpscr = buf[i]; \
> > > +} while (0)
> > > +
> > > +#define unsafe_copy_vsx_from_user(task, from, label) do { \
> > > + struct task_struct *__t = task; \
> > > + u64 __user *__f = (u64 __user *)from; \
> > > + u64 buf[ELF_NVSRHALFREG]; \
> > > + int i; \
> > > + \
> > > + unsafe_copy_from_user(buf, __f, \
> > > + ELF_NVSRHALFREG * sizeof(double), \
> > > + label); \
> > > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> > > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \
> > > +} while (0)
> > > +
> > > +
> > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \
> > > struct task_struct *__t = task; \
> > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \
> > > ELF_NFPREG * sizeof(double), label)
> > >
> > > +#define unsafe_copy_fpr_from_user(task, from, label) \
> > > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \
> > > + ELF_NFPREG * sizeof(double), label)
> > > +
> > > static inline unsigned long
> > > copy_fpr_to_user(void __user *to, struct task_struct *task)
> > > {
> > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
> > > #else
> > > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
> > >
> > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
> > > +
> > > static inline unsigned long
> > > copy_fpr_to_user(void __user *to, struct task_struct *task)
> > > {
> > > --
> > > 2.26.1
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> > MK1 1PT, UK
> > Registration No: 1397386 (Wales)
>
^ permalink raw reply
* RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Christopher M. Riedl @ 2021-02-01 16:55 UTC (permalink / raw)
To: David Laight, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <0433d40adacc47a3a27bc8bc35e076e3@AcuMS.aculab.com>
On Mon Feb 1, 2021 at 10:15 AM CST, David Laight wrote:
> From: Christopher M. Riedl
> > Sent: 01 February 2021 15:56
> >
> > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> > > From: Christopher M. Riedl
> > > > Sent: 28 January 2021 04:04
> > > >
> > > > Reuse the "safe" implementation from signal.c except for calling
> > > > unsafe_copy_from_user() to copy into a local buffer.
> > > >
> > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > > > ---
> > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > > > 1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > > > index 2559a681536e..c18402d625f1 100644
> > > > --- a/arch/powerpc/kernel/signal.h
> > > > +++ b/arch/powerpc/kernel/signal.h
> > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > > &buf[i], label);\
> > > > } while (0)
> > > >
> > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \
> > > > + struct task_struct *__t = task; \
> > > > + u64 __user *__f = (u64 __user *)from; \
> > > > + u64 buf[ELF_NFPREG]; \
> > >
> > > How big is that buffer?
> > > Isn't is likely to be reasonably large compared to a reasonable
> > > kernel stack frame.
> > > Especially since this isn't even a leaf function.
> > >
> >
> > I think Christophe answered this - I don't really have an opinion either
> > way. What would be a 'reasonable' kernel stack frame for reference?
>
> Zero :-)
>
Hehe good point!
> >
> > > > + int i; \
> > > > + \
> > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
> > > > + label); \
> > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > > > + __t->thread.TS_FPR(i) = buf[i]; \
> > > > + __t->thread.fp_state.fpscr = buf[i]; \
> > > > +} while (0)
>
> On further reflection, since you immediately loop through the buffer
> why not just use user_access_begin() and unsafe_get_user() in the loop.
Christophe had suggested this a few revisions ago as well. When I tried
this approach, the signal handling performance took a pretty big hit:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html
I included some numbers on v3 as well but decided to drop the approach
altogether for this one since it just didn't seem worth the hit.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
From: Christoph Hellwig @ 2021-02-01 16:28 UTC (permalink / raw)
To: Miroslav Benes
Cc: Petr Mladek, Jiri Kosina, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Josh Poimboeuf, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
Joe Lawrence, dri-devel, Thomas Zimmermann, Jessica Yu,
Frederic Barrat, Daniel Vetter, linuxppc-dev, Christoph Hellwig
In-Reply-To: <alpine.LSU.2.21.2102011436320.21637@pobox.suse.cz>
On Mon, Feb 01, 2021 at 02:37:12PM +0100, Miroslav Benes wrote:
> > > This change is not needed. (objname == NULL) means that we are
> > > interested only in symbols in "vmlinux".
> > >
> > > module_kallsyms_on_each_symbol(klp_find_callback, &args)
> > > will always fail when objname == NULL.
> >
> > I just tried to keep the old behavior. I can respin it with your
> > recommended change noting the change in behavior, though.
>
> Yes, please. It would be cleaner that way.
Let me know if this works for you:
---
From 18af41e88d088cfb8680d1669fcae2bc2ede5328 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Wed, 20 Jan 2021 16:23:16 +0100
Subject: kallsyms: refactor {,module_}kallsyms_on_each_symbol
Require an explicit call to module_kallsyms_on_each_symbol to look
for symbols in modules instead of the call from kallsyms_on_each_symbol,
and acquire module_mutex inside of module_kallsyms_on_each_symbol instead
of leaving that up to the caller. Note that this slightly changes the
behavior for the livepatch code in that the symbols from vmlinux are not
iterated anymore if objname is set, but that actually is the desired
behavior in this case.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
kernel/kallsyms.c | 6 +++++-
kernel/livepatch/core.c | 2 --
kernel/module.c | 13 ++++---------
3 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index fe9de067771c34..a0d3f0865916f9 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -177,6 +177,10 @@ unsigned long kallsyms_lookup_name(const char *name)
return module_kallsyms_lookup_name(name);
}
+/*
+ * Iterate over all symbols in vmlinux. For symbols from modules use
+ * module_kallsyms_on_each_symbol instead.
+ */
int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
unsigned long),
void *data)
@@ -192,7 +196,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
if (ret != 0)
return ret;
}
- return module_kallsyms_on_each_symbol(fn, data);
+ return 0;
}
static unsigned long get_symbol_pos(unsigned long addr,
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 262cd9b003b9f0..335d988bd81117 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -164,12 +164,10 @@ static int klp_find_object_symbol(const char *objname, const char *name,
.pos = sympos,
};
- mutex_lock(&module_mutex);
if (objname)
module_kallsyms_on_each_symbol(klp_find_callback, &args);
else
kallsyms_on_each_symbol(klp_find_callback, &args);
- mutex_unlock(&module_mutex);
/*
* Ensure an address was found. If sympos is 0, ensure symbol is unique;
diff --git a/kernel/module.c b/kernel/module.c
index 6772fb2680eb3e..25345792c770d1 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -255,11 +255,6 @@ static void mod_update_bounds(struct module *mod)
struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
#endif /* CONFIG_KGDB_KDB */
-static void module_assert_mutex(void)
-{
- lockdep_assert_held(&module_mutex);
-}
-
static void module_assert_mutex_or_preempt(void)
{
#ifdef CONFIG_LOCKDEP
@@ -4379,8 +4374,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
unsigned int i;
int ret;
- module_assert_mutex();
-
+ mutex_lock(&module_mutex);
list_for_each_entry(mod, &modules, list) {
/* We hold module_mutex: no need for rcu_dereference_sched */
struct mod_kallsyms *kallsyms = mod->kallsyms;
@@ -4396,10 +4390,11 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
ret = fn(data, kallsyms_symbol_name(kallsyms, i),
mod, kallsyms_symbol_value(sym));
if (ret != 0)
- return ret;
+ break;
}
}
- return 0;
+ mutex_unlock(&module_mutex);
+ return ret;
}
#endif /* CONFIG_KALLSYMS */
--
2.29.2
^ permalink raw reply related
* RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: David Laight @ 2021-02-01 16:15 UTC (permalink / raw)
To: 'Christopher M. Riedl', linuxppc-dev@lists.ozlabs.org
In-Reply-To: <C8YBET4IGYGF.3QYANVRRHMV0R@geist>
From: Christopher M. Riedl
> Sent: 01 February 2021 15:56
>
> On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> > From: Christopher M. Riedl
> > > Sent: 28 January 2021 04:04
> > >
> > > Reuse the "safe" implementation from signal.c except for calling
> > > unsafe_copy_from_user() to copy into a local buffer.
> > >
> > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > > ---
> > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > > index 2559a681536e..c18402d625f1 100644
> > > --- a/arch/powerpc/kernel/signal.h
> > > +++ b/arch/powerpc/kernel/signal.h
> > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > &buf[i], label);\
> > > } while (0)
> > >
> > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \
> > > + struct task_struct *__t = task; \
> > > + u64 __user *__f = (u64 __user *)from; \
> > > + u64 buf[ELF_NFPREG]; \
> >
> > How big is that buffer?
> > Isn't is likely to be reasonably large compared to a reasonable
> > kernel stack frame.
> > Especially since this isn't even a leaf function.
> >
>
> I think Christophe answered this - I don't really have an opinion either
> way. What would be a 'reasonable' kernel stack frame for reference?
Zero :-)
>
> > > + int i; \
> > > + \
> > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
> > > + label); \
> > > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > > + __t->thread.TS_FPR(i) = buf[i]; \
> > > + __t->thread.fp_state.fpscr = buf[i]; \
> > > +} while (0)
On further reflection, since you immediately loop through the buffer
why not just use user_access_begin() and unsafe_get_user() in the loop.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
From: Christopher M. Riedl @ 2021-02-01 15:55 UTC (permalink / raw)
To: David Laight, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6a6ce1a53fcf4669a9848114d3460fef@AcuMS.aculab.com>
On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> From: Christopher M. Riedl
> > Sent: 28 January 2021 04:04
> >
> > Reuse the "safe" implementation from signal.c except for calling
> > unsafe_copy_from_user() to copy into a local buffer.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > index 2559a681536e..c18402d625f1 100644
> > --- a/arch/powerpc/kernel/signal.h
> > +++ b/arch/powerpc/kernel/signal.h
> > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > &buf[i], label);\
> > } while (0)
> >
> > +#define unsafe_copy_fpr_from_user(task, from, label) do { \
> > + struct task_struct *__t = task; \
> > + u64 __user *__f = (u64 __user *)from; \
> > + u64 buf[ELF_NFPREG]; \
>
> How big is that buffer?
> Isn't is likely to be reasonably large compared to a reasonable
> kernel stack frame.
> Especially since this isn't even a leaf function.
>
I think Christophe answered this - I don't really have an opinion either
way. What would be a 'reasonable' kernel stack frame for reference?
> > + int i; \
> > + \
> > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \
>
> That really ought to be sizeof(buf).
>
Agreed, I will fix this. Thanks!
> David
>
>
> > + label); \
> > + for (i = 0; i < ELF_NFPREG - 1; i++) \
> > + __t->thread.TS_FPR(i) = buf[i]; \
> > + __t->thread.fp_state.fpscr = buf[i]; \
> > +} while (0)
> > +
> > +#define unsafe_copy_vsx_from_user(task, from, label) do { \
> > + struct task_struct *__t = task; \
> > + u64 __user *__f = (u64 __user *)from; \
> > + u64 buf[ELF_NVSRHALFREG]; \
> > + int i; \
> > + \
> > + unsafe_copy_from_user(buf, __f, \
> > + ELF_NVSRHALFREG * sizeof(double), \
> > + label); \
> > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \
> > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \
> > +} while (0)
> > +
> > +
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \
> > struct task_struct *__t = task; \
> > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \
> > ELF_NFPREG * sizeof(double), label)
> >
> > +#define unsafe_copy_fpr_from_user(task, from, label) \
> > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \
> > + ELF_NFPREG * sizeof(double), label)
> > +
> > static inline unsigned long
> > copy_fpr_to_user(void __user *to, struct task_struct *task)
> > {
> > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
> > #else
> > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
> >
> > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
> > +
> > static inline unsigned long
> > copy_fpr_to_user(void __user *to, struct task_struct *task)
> > {
> > --
> > 2.26.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
From: Miroslav Benes @ 2021-02-01 14:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Petr Mladek, Jiri Kosina, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Josh Poimboeuf, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
Joe Lawrence, dri-devel, Thomas Zimmermann, Jessica Yu,
Frederic Barrat, Daniel Vetter, linuxppc-dev
In-Reply-To: <20210128181421.2279-6-hch@lst.de>
One more thing...
> @@ -4379,8 +4379,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> unsigned int i;
> int ret;
>
> - module_assert_mutex();
> -
> + mutex_lock(&module_mutex);
> list_for_each_entry(mod, &modules, list) {
> /* We hold module_mutex: no need for rcu_dereference_sched */
> struct mod_kallsyms *kallsyms = mod->kallsyms;
This was the last user of module_assert_mutex(), which can be removed now.
Miroslav
^ permalink raw reply
* Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
From: Miroslav Benes @ 2021-02-01 13:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Petr Mladek, Jiri Kosina, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Josh Poimboeuf, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
Joe Lawrence, dri-devel, Thomas Zimmermann, Jessica Yu,
Frederic Barrat, Daniel Vetter, linuxppc-dev
In-Reply-To: <20210201114749.GB19696@lst.de>
On Mon, 1 Feb 2021, Christoph Hellwig wrote:
> On Fri, Jan 29, 2021 at 10:43:36AM +0100, Petr Mladek wrote:
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -164,12 +164,8 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> > > .pos = sympos,
> > > };
> > >
> > > - mutex_lock(&module_mutex);
> > > - if (objname)
> > > + if (objname || !kallsyms_on_each_symbol(klp_find_callback, &args))
> > > module_kallsyms_on_each_symbol(klp_find_callback, &args);
> > > - else
> > > - kallsyms_on_each_symbol(klp_find_callback, &args);
> > > - mutex_unlock(&module_mutex);
> >
> > This change is not needed. (objname == NULL) means that we are
> > interested only in symbols in "vmlinux".
> >
> > module_kallsyms_on_each_symbol(klp_find_callback, &args)
> > will always fail when objname == NULL.
>
> I just tried to keep the old behavior. I can respin it with your
> recommended change noting the change in behavior, though.
Yes, please. It would be cleaner that way.
Miroslav
^ permalink raw reply
* Re: [PATCH 04/13] module: use RCU to synchronize find_module
From: Miroslav Benes @ 2021-02-01 13:16 UTC (permalink / raw)
To: Jessica Yu
Cc: Petr Mladek, Joe Lawrence, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Jiri Kosina, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
dri-devel, Thomas Zimmermann, Josh Poimboeuf, Frederic Barrat,
Daniel Vetter, linuxppc-dev, Christoph Hellwig
In-Reply-To: <YBfvvdna9pSeu+1g@gunter>
On Mon, 1 Feb 2021, Jessica Yu wrote:
> +++ Miroslav Benes [29/01/21 16:29 +0100]:
> >On Thu, 28 Jan 2021, Christoph Hellwig wrote:
> >
> >> Allow for a RCU-sched critical section around find_module, following
> >> the lower level find_module_all helper, and switch the two callers
> >> outside of module.c to use such a RCU-sched critical section instead
> >> of module_mutex.
> >
> >That's a nice idea.
> >
> >> @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object
> >> *obj)
> >> if (!klp_is_module(obj))
> >> return;
> >>
> >> - mutex_lock(&module_mutex);
> >> + rcu_read_lock_sched();
> >> /*
> >> * We do not want to block removal of patched modules and therefore
> >> * we do not take a reference here. The patches are removed by
> >> @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object
> >> *obj)
> >> if (mod && mod->klp_alive)
> >
> >RCU always baffles me a bit, so I'll ask. Don't we need
> >rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I
> >wonder.
>
> Same here :-) I had to double check the RCU documentation. For our
> modules list case I believe the rcu list API should take care of that
> for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt:
>
> rcu_dereference() is typically used indirectly, via the _rcu
> list-manipulation primitives, such as list_for_each_entry_rcu()
Ok, thanks to both for checking and explanation.
Ack to the patch then.
Miroslav
^ permalink raw reply
* Re: [RFC 00/20] TLB batching consolidation and enhancements
From: Peter Zijlstra @ 2021-02-01 12:44 UTC (permalink / raw)
To: Nadav Amit
Cc: Andrea Arcangeli, linux-s390, X86 ML, Yu Zhao, Will Deacon,
Mel Gorman, Dave Hansen, LKML, Nicholas Piggin, Linux-MM,
linux-csky@vger.kernel.org, Andy Lutomirski, Andrew Morton,
linuxppc-dev, Thomas Gleixner
In-Reply-To: <A1589669-34AE-4E15-8358-79BAD7C72520@vmware.com>
On Sun, Jan 31, 2021 at 07:57:01AM +0000, Nadav Amit wrote:
> > On Jan 30, 2021, at 7:30 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > I'll go through the patches a bit more closely when they all come
> > through. Sparc and powerpc of course need the arch lazy mode to get
> > per-page/pte information for operations that are not freeing pages,
> > which is what mmu gather is designed for.
>
> IIUC you mean any PTE change requires a TLB flush. Even setting up a new PTE
> where no previous PTE was set, right?
These are the HASH architectures. Their hardware doesn't walk the
page-tables, but it consults a hash-table to resolve page translations.
They _MUST_ flush the entries under the PTL to avoid ever seeing
conflicting information, which will make them really unhappy. They can
do this because they have TLBI broadcast.
There's a few more details I'm sure, but those seem to have slipped from
my mind.
^ permalink raw reply
* RE: [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
From: PLATTNER Christoph @ 2021-02-01 12:35 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: christoph.plattner@gmx.at, KOENIG Werner, HAMETNER Reinhard,
linux-kernel@vger.kernel.org, REITHER Robert - Contractor,
PLATTNER Christoph, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <035a7cde-7ffd-5f27-81e1-a8d3648e4c1c@csgroup.eu>
Thank you very much, I appreciate your fast responses.
Thank you also for clarification, I did completely oversee
the permission settings in the segment setup and expected
the fault reaction on the PP bits in the TLB.
And I will re-read the chapters, got get deeper into this topic.
Greetings
Christoph
-----Original Message-----
From: Christophe Leroy <christophe.leroy@csgroup.eu>
Sent: Montag, 1. Februar 2021 12:39
To: PLATTNER Christoph <christoph.plattner@thalesgroup.com>; Benjamin Herrenschmidt <benh@kernel.crashing.org>; Paul Mackerras <paulus@samba.org>; Michael Ellerman <mpe@ellerman.id.au>
Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; HAMETNER Reinhard <reinhard.hametner@thalesgroup.com>; REITHER Robert - Contractor <robert.reither@external.thalesgroup.com>; KOENIG Werner <werner.koenig@thalesgroup.com>
Subject: Re: [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
Le 01/02/2021 à 11:22, PLATTNER Christoph a écrit :
> Hello to all, and thank you very much for first and second fast response.
>
> I do not have a long history on PowerPC MMU environment, I hacked into
> this topic for about 3 months for analyzing that problem- so, sorry, if I am wrong in some points ...
Yes you are wrong on some points, sorry, see below.
>
> What I learn so far from this MPC5121e (variant of e300c4 core):
> - It uses book3s32 hash-code, but it DOES NOT provide KEY hash method, so always the
> branch "if (! Hash) ...." is taken, so, I assume that "key 0" and "key 1" setups are not
> used on this CPU (not supporting MMU_FTR_HPTE_TABLE)
hash method is not used, this is SW TLB loading that is used, but still, all the PP and Ks/Kp keys defined in the segment register are used, see e300 core reference manual §6.4.2 Page Memory Protection
> - The PP bits are NOT checked by the CPU in HW, even if set to 00, the CPU does not react.
> As far I have understood, the TLB miss routines are responsible for checking permissions.
> The TLB miss routines check the Linux PTE styled entries and generates the PP bits
> for the TLB entry. The PowerPC PP bits are never check elsewhere on that CPU models ...
PP bits ARE checked hoppefully. If it was not the case, then the TLB miss routines would install a TLB on a read, then the user could do a write without any verification being done ?
Refer to e300 Core reference Manual, §6.1.4 Memory Protection Facilities
As I explained in the patch, the problem is not that the HW doesn't check the permission. It is that user accessed been done with key 0 as programmed in the segment registers, PP 00 means RW access.
> - The PTE entries in Linux are fully "void" in sense of this CPU type, as this CPU does not
> read any PTEs from RAM (no HW support in contrast to x86 or ARM or later ppc...).
No, the PTE are read by the TLB miss exception handlers and writen into TLB entries.
>
> In summary - as far as I understand it now - we have to handle the PTE
> bits differently (Linux style) for PROT_NONE permissions - OR - we
> have to expand the permission checking like my proposed experimental
> patch. (PROT_NONE is not NUMA related only, but may not used very often ...).
Yes, expanding the permission checking is the easiest solution, hence the patch I sent out based on your proposal.
>
> Another related point:
> According e300 RM (manual) the ACCESSED bit in the PTE shall be set on
> TLB miss, as it is an indication, that page is used. In 4.4 kernel
> this write back of the _PAGE_ACCESSED bit was performed after successful permission check:
>
> bne- DataAddressInvalid /* return if access not permitted */
> ori r0,r0,_PAGE_ACCESSED /* set _PAGE_ACCESSED in pte */
> /*
> * NOTE! We are assuming this is not an SMP system, otherwise
> * we would need to update the pte atomically with lwarx/stwcx.
> */
> stw r0,0(r2) /* update PTE (accessed bit) */
> /* Convert linux-style PTE to low word of PPC-style PTE */
>
> Bit is set (ori ...) and written back (stw ...) to Linux PTE. May be,
> this is not needed, as the PTE is never seen by the PPC chip. But I do
> not understand, WHY the PAGE_ACCCESSED is used for permission check in the late 5.4 kernel (not used in 4.4 kernel):
>
> cmplw 0,r1,r3
> mfspr r2, SPRN_SDR1
> li r1, _PAGE_PRESENT | _PAGE_ACCESSED
> rlwinm r2, r2, 28, 0xfffff000
> bgt- 112f
>
> What is the reason or relevance for checking this here ?
> Was not checked in 4.4, bit or-ed afterwards, as it is accessed now.
> Do you know the reason of change on this point ?
PAGE_ACCESSED is important for memory management, linux kernel need it.
But instead of spending time at every miss to perform a write which will be a no-op in 99% of cases, we prefer bailing out to the page_fault logic when the accessed bit is not set. Then the page_fault logic will set the bit.
This also allowed to simplify the handling in __set_pte()_at function by avoiding races in the update of PTEs.
>
> Another remark to Core manual relevant for this:
> There is the reference manual for e300 core available (e300 RM). It includes
> many remarks in range of Memory Management section, that many features
> are optional or variable for dedicated implementations. On the other hand,
> the MPC5121e reference manual refers to the e300 core RM, but DOES NOT
> information, which of the optional points are there or nor. According my
> analysis, MPC5121e does not include any of the optional features.
>
Not sure what you mean. As far as I understand, that chapter tells you that some functionnalities
are optional for the powerpc architectecture, and provided (or not) by the e300 core. The MPC5121
supports all the things that are defined by e300 core.
>
> Thanks a lot for first reactions
You are welcome, don't hesitate if you have additional questions.
Christophe
^ permalink raw reply
* Re: [PATCH 04/13] module: use RCU to synchronize find_module
From: Jessica Yu @ 2021-02-01 12:10 UTC (permalink / raw)
To: Miroslav Benes
Cc: Petr Mladek, Joe Lawrence, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Jiri Kosina, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
dri-devel, Thomas Zimmermann, Josh Poimboeuf, Frederic Barrat,
Daniel Vetter, linuxppc-dev, Christoph Hellwig
In-Reply-To: <alpine.LSU.2.21.2101291626080.22237@pobox.suse.cz>
+++ Miroslav Benes [29/01/21 16:29 +0100]:
>On Thu, 28 Jan 2021, Christoph Hellwig wrote:
>
>> Allow for a RCU-sched critical section around find_module, following
>> the lower level find_module_all helper, and switch the two callers
>> outside of module.c to use such a RCU-sched critical section instead
>> of module_mutex.
>
>That's a nice idea.
>
>> @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object *obj)
>> if (!klp_is_module(obj))
>> return;
>>
>> - mutex_lock(&module_mutex);
>> + rcu_read_lock_sched();
>> /*
>> * We do not want to block removal of patched modules and therefore
>> * we do not take a reference here. The patches are removed by
>> @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj)
>> if (mod && mod->klp_alive)
>
>RCU always baffles me a bit, so I'll ask. Don't we need
>rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I
>wonder.
Same here :-) I had to double check the RCU documentation. For our
modules list case I believe the rcu list API should take care of that
for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt:
rcu_dereference() is typically used indirectly, via the _rcu
list-manipulation primitives, such as list_for_each_entry_rcu()
^ permalink raw reply
* Re: [RFC 11/20] mm/tlb: remove arch-specific tlb_start/end_vma()
From: Peter Zijlstra @ 2021-02-01 12:09 UTC (permalink / raw)
To: Nadav Amit
Cc: Andrea Arcangeli, linux-s390, x86, Yu Zhao, linuxppc-dev,
Dave Hansen, linux-kernel, Nick Piggin, linux-mm, Nadav Amit,
linux-csky, Andy Lutomirski, Andrew Morton, Will Deacon,
Thomas Gleixner
In-Reply-To: <20210131001132.3368247-12-namit@vmware.com>
On Sat, Jan 30, 2021 at 04:11:23PM -0800, Nadav Amit wrote:
> diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
> index 427bfcc6cdec..b97136b7010b 100644
> --- a/include/asm-generic/tlb.h
> +++ b/include/asm-generic/tlb.h
> @@ -334,8 +334,8 @@ static inline void __tlb_reset_range(struct mmu_gather *tlb)
>
> #ifdef CONFIG_MMU_GATHER_NO_RANGE
>
> -#if defined(tlb_flush) || defined(tlb_start_vma) || defined(tlb_end_vma)
> -#error MMU_GATHER_NO_RANGE relies on default tlb_flush(), tlb_start_vma() and tlb_end_vma()
> +#if defined(tlb_flush)
> +#error MMU_GATHER_NO_RANGE relies on default tlb_flush()
> #endif
>
> /*
> @@ -362,10 +362,6 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
>
> #ifndef tlb_flush
>
> -#if defined(tlb_start_vma) || defined(tlb_end_vma)
> -#error Default tlb_flush() relies on default tlb_start_vma() and tlb_end_vma()
> -#endif
#ifdef CONFIG_ARCH_WANT_AGGRESSIVE_TLB_FLUSH_BATCHING
#error ....
#endif
goes here...
> static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
> {
> if (tlb->fullmm)
> return;
>
> + if (IS_ENABLED(CONFIG_ARCH_WANT_AGGRESSIVE_TLB_FLUSH_BATCHING))
> + return;
Also, can you please stick to the CONFIG_MMU_GATHER_* namespace?
I also don't think AGRESSIVE_FLUSH_BATCHING quite captures what it does.
How about:
CONFIG_MMU_GATHER_NO_PER_VMA_FLUSH
?
^ permalink raw reply
* Re: [PATCH 05/13] kallsyms: refactor {,module_}kallsyms_on_each_symbol
From: Christoph Hellwig @ 2021-02-01 11:47 UTC (permalink / raw)
To: Petr Mladek
Cc: Jiri Kosina, Andrew Donnellan, linux-kbuild, David Airlie,
Masahiro Yamada, Josh Poimboeuf, Maarten Lankhorst, linux-kernel,
Maxime Ripard, live-patching, Michal Marek, Joe Lawrence,
dri-devel, Thomas Zimmermann, Jessica Yu, Frederic Barrat,
Daniel Vetter, Miroslav Benes, linuxppc-dev, Christoph Hellwig
In-Reply-To: <YBPYyEvesLMrRtZM@alley>
On Fri, Jan 29, 2021 at 10:43:36AM +0100, Petr Mladek wrote:
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -164,12 +164,8 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> > .pos = sympos,
> > };
> >
> > - mutex_lock(&module_mutex);
> > - if (objname)
> > + if (objname || !kallsyms_on_each_symbol(klp_find_callback, &args))
> > module_kallsyms_on_each_symbol(klp_find_callback, &args);
> > - else
> > - kallsyms_on_each_symbol(klp_find_callback, &args);
> > - mutex_unlock(&module_mutex);
>
> This change is not needed. (objname == NULL) means that we are
> interested only in symbols in "vmlinux".
>
> module_kallsyms_on_each_symbol(klp_find_callback, &args)
> will always fail when objname == NULL.
I just tried to keep the old behavior. I can respin it with your
recommended change noting the change in behavior, though.
^ permalink raw reply
* Re: [PATCH 04/13] module: use RCU to synchronize find_module
From: Christoph Hellwig @ 2021-02-01 11:46 UTC (permalink / raw)
To: Miroslav Benes
Cc: Petr Mladek, Jiri Kosina, Andrew Donnellan, linux-kbuild,
David Airlie, Masahiro Yamada, Josh Poimboeuf, Maarten Lankhorst,
linux-kernel, Maxime Ripard, live-patching, Michal Marek,
Joe Lawrence, dri-devel, Thomas Zimmermann, Jessica Yu,
Frederic Barrat, Daniel Vetter, linuxppc-dev, Christoph Hellwig
In-Reply-To: <alpine.LSU.2.21.2101291626080.22237@pobox.suse.cz>
On Fri, Jan 29, 2021 at 04:29:02PM +0100, Miroslav Benes wrote:
> >
> > - mutex_lock(&module_mutex);
> > + rcu_read_lock_sched();
> > /*
> > * We do not want to block removal of patched modules and therefore
> > * we do not take a reference here. The patches are removed by
> > @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj)
> > if (mod && mod->klp_alive)
>
> RCU always baffles me a bit, so I'll ask. Don't we need
> rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I
> wonder.
rcu_dereference* is only used for dereferencing points where that
reference itself is RCU protected, that is the lookup of mod itself down
in find_module_all in this case.
^ permalink raw reply
* Re: [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
From: Christophe Leroy @ 2021-02-01 11:39 UTC (permalink / raw)
To: PLATTNER Christoph, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: REITHER Robert - Contractor, HAMETNER Reinhard, KOENIG Werner,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <1b194840-d4e6-4660-94d9-6bac623442cf@THSDC1IRIMBX13P.iris.infra.thales>
Le 01/02/2021 à 11:22, PLATTNER Christoph a écrit :
> Hello to all, and thank you very much for first and second fast response.
>
> I do not have a long history on PowerPC MMU environment, I hacked into this topic
> for about 3 months for analyzing that problem- so, sorry, if I am wrong in some points ...
Yes you are wrong on some points, sorry, see below.
>
> What I learn so far from this MPC5121e (variant of e300c4 core):
> - It uses book3s32 hash-code, but it DOES NOT provide KEY hash method, so always the
> branch "if (! Hash) ...." is taken, so, I assume that "key 0" and "key 1" setups are not
> used on this CPU (not supporting MMU_FTR_HPTE_TABLE)
hash method is not used, this is SW TLB loading that is used, but still, all the PP and Ks/Kp keys
defined in the segment register are used, see e300 core reference manual §6.4.2 Page Memory Protection
> - The PP bits are NOT checked by the CPU in HW, even if set to 00, the CPU does not react.
> As far I have understood, the TLB miss routines are responsible for checking permissions.
> The TLB miss routines check the Linux PTE styled entries and generates the PP bits
> for the TLB entry. The PowerPC PP bits are never check elsewhere on that CPU models ...
PP bits ARE checked hoppefully. If it was not the case, then the TLB miss routines would install a
TLB on a read, then the user could do a write without any verification being done ?
Refer to e300 Core reference Manual, §6.1.4 Memory Protection Facilities
As I explained in the patch, the problem is not that the HW doesn't check the permission. It is that
user accessed been done with key 0 as programmed in the segment registers, PP 00 means RW access.
> - The PTE entries in Linux are fully "void" in sense of this CPU type, as this CPU does not
> read any PTEs from RAM (no HW support in contrast to x86 or ARM or later ppc...).
No, the PTE are read by the TLB miss exception handlers and writen into TLB entries.
>
> In summary - as far as I understand it now - we have to handle the PTE bits differently
> (Linux style) for PROT_NONE permissions - OR - we have to expand the permission
> checking like my proposed experimental patch. (PROT_NONE is not NUMA related only,
> but may not used very often ...).
Yes, expanding the permission checking is the easiest solution, hence the patch I sent out based on
your proposal.
>
> Another related point:
> According e300 RM (manual) the ACCESSED bit in the PTE shall be set on TLB miss, as
> it is an indication, that page is used. In 4.4 kernel this write back of the _PAGE_ACCESSED
> bit was performed after successful permission check:
>
> bne- DataAddressInvalid /* return if access not permitted */
> ori r0,r0,_PAGE_ACCESSED /* set _PAGE_ACCESSED in pte */
> /*
> * NOTE! We are assuming this is not an SMP system, otherwise
> * we would need to update the pte atomically with lwarx/stwcx.
> */
> stw r0,0(r2) /* update PTE (accessed bit) */
> /* Convert linux-style PTE to low word of PPC-style PTE */
>
> Bit is set (ori ...) and written back (stw ...) to Linux PTE. May be, this is not needed, as the
> PTE is never seen by the PPC chip. But I do not understand, WHY the PAGE_ACCCESSED
> is used for permission check in the late 5.4 kernel (not used in 4.4 kernel):
>
> cmplw 0,r1,r3
> mfspr r2, SPRN_SDR1
> li r1, _PAGE_PRESENT | _PAGE_ACCESSED
> rlwinm r2, r2, 28, 0xfffff000
> bgt- 112f
>
> What is the reason or relevance for checking this here ?
> Was not checked in 4.4, bit or-ed afterwards, as it is accessed now.
> Do you know the reason of change on this point ?
PAGE_ACCESSED is important for memory management, linux kernel need it.
But instead of spending time at every miss to perform a write which will be a no-op in 99% of cases,
we prefer bailing out to the page_fault logic when the accessed bit is not set. Then the page_fault
logic will set the bit.
This also allowed to simplify the handling in __set_pte()_at function by avoiding races in the
update of PTEs.
>
> Another remark to Core manual relevant for this:
> There is the reference manual for e300 core available (e300 RM). It includes
> many remarks in range of Memory Management section, that many features
> are optional or variable for dedicated implementations. On the other hand,
> the MPC5121e reference manual refers to the e300 core RM, but DOES NOT
> information, which of the optional points are there or nor. According my
> analysis, MPC5121e does not include any of the optional features.
>
Not sure what you mean. As far as I understand, that chapter tells you that some functionnalities
are optional for the powerpc architectecture, and provided (or not) by the e300 core. The MPC5121
supports all the things that are defined by e300 core.
>
> Thanks a lot for first reactions
You are welcome, don't hesitate if you have additional questions.
Christophe
^ permalink raw reply
* RE: [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
From: PLATTNER Christoph @ 2021-02-01 10:22 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: KOENIG Werner, HAMETNER Reinhard, linux-kernel@vger.kernel.org,
REITHER Robert - Contractor, PLATTNER Christoph,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4a0c6e3bb8f0c162457bf54d9bc6fd8d7b55129f.1612160907.git.christophe.leroy@csgroup.eu>
Hello to all, and thank you very much for first and second fast response.
I do not have a long history on PowerPC MMU environment, I hacked into this topic
for about 3 months for analyzing that problem- so, sorry, if I am wrong in some points ...
What I learn so far from this MPC5121e (variant of e300c4 core):
- It uses book3s32 hash-code, but it DOES NOT provide KEY hash method, so always the
branch "if (! Hash) ...." is taken, so, I assume that "key 0" and "key 1" setups are not
used on this CPU (not supporting MMU_FTR_HPTE_TABLE)
- The PP bits are NOT checked by the CPU in HW, even if set to 00, the CPU does not react.
As far I have understood, the TLB miss routines are responsible for checking permissions.
The TLB miss routines check the Linux PTE styled entries and generates the PP bits
for the TLB entry. The PowerPC PP bits are never check elsewhere on that CPU models ...
- The PTE entries in Linux are fully "void" in sense of this CPU type, as this CPU does not
read any PTEs from RAM (no HW support in contrast to x86 or ARM or later ppc...).
In summary - as far as I understand it now - we have to handle the PTE bits differently
(Linux style) for PROT_NONE permissions - OR - we have to expand the permission
checking like my proposed experimental patch. (PROT_NONE is not NUMA related only,
but may not used very often ...).
Another related point:
According e300 RM (manual) the ACCESSED bit in the PTE shall be set on TLB miss, as
it is an indication, that page is used. In 4.4 kernel this write back of the _PAGE_ACCESSED
bit was performed after successful permission check:
bne- DataAddressInvalid /* return if access not permitted */
ori r0,r0,_PAGE_ACCESSED /* set _PAGE_ACCESSED in pte */
/*
* NOTE! We are assuming this is not an SMP system, otherwise
* we would need to update the pte atomically with lwarx/stwcx.
*/
stw r0,0(r2) /* update PTE (accessed bit) */
/* Convert linux-style PTE to low word of PPC-style PTE */
Bit is set (ori ...) and written back (stw ...) to Linux PTE. May be, this is not needed, as the
PTE is never seen by the PPC chip. But I do not understand, WHY the PAGE_ACCCESSED
is used for permission check in the late 5.4 kernel (not used in 4.4 kernel):
cmplw 0,r1,r3
mfspr r2, SPRN_SDR1
li r1, _PAGE_PRESENT | _PAGE_ACCESSED
rlwinm r2, r2, 28, 0xfffff000
bgt- 112f
What is the reason or relevance for checking this here ?
Was not checked in 4.4, bit or-ed afterwards, as it is accessed now.
Do you know the reason of change on this point ?
Another remark to Core manual relevant for this:
There is the reference manual for e300 core available (e300 RM). It includes
many remarks in range of Memory Management section, that many features
are optional or variable for dedicated implementations. On the other hand,
the MPC5121e reference manual refers to the e300 core RM, but DOES NOT
information, which of the optional points are there or nor. According my
analysis, MPC5121e does not include any of the optional features.
Thanks a lot for first reactions
Christoph
-----Original Message-----
From: Christophe Leroy <christophe.leroy@csgroup.eu>
Sent: Montag, 1. Februar 2021 07:30
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>; Paul Mackerras <paulus@samba.org>; Michael Ellerman <mpe@ellerman.id.au>; PLATTNER Christoph <christoph.plattner@thalesgroup.com>
Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
Subject: [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
On book3s/32, page protection is defined by the PP bits in the PTE which provide the following protection depending on the access keys defined in the matching segment register:
- PP 00 means RW with key 0 and N/A with key 1.
- PP 01 means RW with key 0 and RO with key 1.
- PP 10 means RW with both key 0 and key 1.
- PP 11 means RO with both key 0 and key 1.
Since the implementation of kernel userspace access protection, PP bits have been set as follows:
- PP00 for pages without _PAGE_USER
- PP01 for pages with _PAGE_USER and _PAGE_RW
- PP11 for pages with _PAGE_USER and without _PAGE_RW
For kernelspace segments, kernel accesses are performed with key 0 and user accesses are performed with key 1. As PP00 is used for non _PAGE_USER pages, user can't access kernel pages not flagged _PAGE_USER while kernel can.
For userspace segments, both kernel and user accesses are performed with key 0, therefore pages not flagged _PAGE_USER are still accessible to the user.
This shouldn't be an issue, because userspace is expected to be accessible to the user. But unlike most other architectures, powerpc implements PROT_NONE protection by removing _PAGE_USER flag instead of flagging the page as not valid. This means that pages in userspace that are not flagged _PAGE_USER shall remain inaccessible.
To get the expected behaviour, just mimic other architectures in the TLB miss handler by checking _PAGE_USER permission on userspace accesses as if it was the _PAGE_PRESENT bit.
Note that this problem only is only for 603 cores. The 604+ have an hash table, and hash_page() function already implement the verification of _PAGE_USER permission on userspace pages.
Reported-by: Christoph Plattner <christoph.plattner@thalesgroup.com>
Fixes: f342adca3afc ("powerpc/32s: Prepare Kernel Userspace Access Protection")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/head_book3s_32.S | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/head_book3s_32.S b/arch/powerpc/kernel/head_book3s_32.S
index 858fbc8b19f3..0004e8a6a58e 100644
--- a/arch/powerpc/kernel/head_book3s_32.S
+++ b/arch/powerpc/kernel/head_book3s_32.S
@@ -453,11 +453,12 @@ InstructionTLBMiss:
cmplw 0,r1,r3
#endif
mfspr r2, SPRN_SDR1
- li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
+ li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
#ifdef CONFIG_MODULES
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
#endif
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
@@ -516,10 +517,11 @@ DataLoadTLBMiss:
lis r1, TASK_SIZE@h /* check if kernel address */
cmplw 0,r1,r3
mfspr r2, SPRN_SDR1
- li r1, _PAGE_PRESENT | _PAGE_ACCESSED
+ li r1, _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1, _PAGE_PRESENT | _PAGE_ACCESSED
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
lwz r2,0(r2) /* get pmd entry */
@@ -593,10 +595,11 @@ DataStoreTLBMiss:
lis r1, TASK_SIZE@h /* check if kernel address */
cmplw 0,r1,r3
mfspr r2, SPRN_SDR1
- li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED
+ li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
lwz r2,0(r2) /* get pmd entry */
--
2.25.0
^ permalink raw reply related
* Re: FSL P5040: KVM HV doesn't work with the RC5 of kernel 5.11
From: Christian Zigotzky @ 2021-02-01 8:29 UTC (permalink / raw)
To: linuxppc-dev, tglx, kvm-ppc@vger.kernel.org
Cc: Darren Stevens, mad skateman, R.T.Dickinson
In-Reply-To: <b0ba0690-507f-7660-79e2-5268cc6684bf@xenosoft.de>
Hello,
I compiled the RC6 of kernel 5.11 today and KVM HV works again.
Therefore I don't need the patch below anymore.
Many thanks for solving the issue,
Christian
On 27 January 2021 at 05:07pm, Christian Zigotzky wrote:
> Hello,
>
> I compiled the RC5 of kernel 5.11 today. Unfortunately KVM HV doesn't
> work anymore on my FSL P5040 board [1]. I tested it with QEMU 5.0.0
> today [2]. The virtual e5500 QEMU machine works with the "RC4 with KVM
> HV" and with the "RC5 without KVM HV". The complete system freezes if
> I use KVM HV with the RC5.
>
> I have bisected and 785025820a6a565185ce9d47fdd8d23dbf91dee8
> (powerpc/mm/highmem: use __set_pte_at() for kmap_local()) [3] is the
> first bad commit.
>
> I was able to revert this bad commit and after a new compiling, KVM HV
> works again. I created a patch for reverting the commit. [4] Please
> find attached the kernel config. I use one uImage for the virtual
> machine and for the P5040 board.
>
> Please check the first bad commit.
>
> Thanks,
> Christian
>
>
> [1] http://wiki.amiga.org/index.php?title=X5000
> [2] qemu-system-ppc64 -M ppce500 -cpu e5500 -enable-kvm -m 1024
> -kernel uImage-5.11 -drive
> format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev
> user,id=mynet0 -device e1000,netdev=mynet0 -append "rw root=/dev/vda"
> -device virtio-vga -device virtio-mouse-pci -device
> virtio-keyboard-pci -device pci-ohci,id=newusb -device
> usb-audio,bus=newusb.0 -smp 4
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.11-rc5&id=785025820a6a565185ce9d47fdd8d23dbf91dee8
> [4]
> diff -rupN a/arch/powerpc/include/asm/highmem.h
> b/arch/powerpc/include/asm/highmem.h
> --- a/arch/powerpc/include/asm/highmem.h 2021-01-27
> 16:12:40.382164118 +0100
> +++ b/arch/powerpc/include/asm/highmem.h 2021-01-27
> 16:10:54.055249957 +0100
> @@ -58,8 +58,6 @@ extern pte_t *pkmap_page_table;
>
> #define flush_cache_kmaps() flush_cache_all()
>
> -#define arch_kmap_local_set_pte(mm, vaddr, ptep, ptev) \
> - __set_pte_at(mm, vaddr, ptep, ptev, 1)
> #define arch_kmap_local_post_map(vaddr, pteval) \
> local_flush_tlb_page(NULL, vaddr)
> #define arch_kmap_local_post_unmap(vaddr) \
^ permalink raw reply
* [PATCH] ASoC: fsl_xcvr: remove unneeded semicolon
From: Yang Li @ 2021-02-01 8:08 UTC (permalink / raw)
To: timur
Cc: alsa-devel, linuxppc-dev, Xiubo.Lee, festevam, tiwai, lgirdwood,
perex, nicoleotsuka, broonie, Yang Li, p.zabel, shengjiu.wang,
linux-kernel
Eliminate the following coccicheck warning:
./sound/soc/fsl/fsl_xcvr.c:739:2-3: Unneeded semicolon
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
---
sound/soc/fsl/fsl_xcvr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
index 3d58c88..65b388a 100644
--- a/sound/soc/fsl/fsl_xcvr.c
+++ b/sound/soc/fsl/fsl_xcvr.c
@@ -736,7 +736,7 @@ static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr)
/* clean current page, including data memory */
memset_io(xcvr->ram_addr, 0, size);
}
- };
+ }
err_firmware:
release_firmware(fw);
--
1.8.3.1
^ permalink raw reply related
* [PATCH] powerpc/603: Fix protection of user pages mapped with PROT_NONE
From: Christophe Leroy @ 2021-02-01 6:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Christoph Plattner
Cc: linuxppc-dev, linux-kernel
On book3s/32, page protection is defined by the PP bits in the PTE
which provide the following protection depending on the access
keys defined in the matching segment register:
- PP 00 means RW with key 0 and N/A with key 1.
- PP 01 means RW with key 0 and RO with key 1.
- PP 10 means RW with both key 0 and key 1.
- PP 11 means RO with both key 0 and key 1.
Since the implementation of kernel userspace access protection,
PP bits have been set as follows:
- PP00 for pages without _PAGE_USER
- PP01 for pages with _PAGE_USER and _PAGE_RW
- PP11 for pages with _PAGE_USER and without _PAGE_RW
For kernelspace segments, kernel accesses are performed with key 0
and user accesses are performed with key 1. As PP00 is used for
non _PAGE_USER pages, user can't access kernel pages not flagged
_PAGE_USER while kernel can.
For userspace segments, both kernel and user accesses are performed
with key 0, therefore pages not flagged _PAGE_USER are still
accessible to the user.
This shouldn't be an issue, because userspace is expected to be
accessible to the user. But unlike most other architectures, powerpc
implements PROT_NONE protection by removing _PAGE_USER flag instead of
flagging the page as not valid. This means that pages in userspace
that are not flagged _PAGE_USER shall remain inaccessible.
To get the expected behaviour, just mimic other architectures in the
TLB miss handler by checking _PAGE_USER permission on userspace
accesses as if it was the _PAGE_PRESENT bit.
Note that this problem only is only for 603 cores. The 604+ have
an hash table, and hash_page() function already implement the
verification of _PAGE_USER permission on userspace pages.
Reported-by: Christoph Plattner <christoph.plattner@thalesgroup.com>
Fixes: f342adca3afc ("powerpc/32s: Prepare Kernel Userspace Access Protection")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/head_book3s_32.S | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/head_book3s_32.S b/arch/powerpc/kernel/head_book3s_32.S
index 858fbc8b19f3..0004e8a6a58e 100644
--- a/arch/powerpc/kernel/head_book3s_32.S
+++ b/arch/powerpc/kernel/head_book3s_32.S
@@ -453,11 +453,12 @@ InstructionTLBMiss:
cmplw 0,r1,r3
#endif
mfspr r2, SPRN_SDR1
- li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
+ li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
#ifdef CONFIG_MODULES
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1,_PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
#endif
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
@@ -516,10 +517,11 @@ DataLoadTLBMiss:
lis r1, TASK_SIZE@h /* check if kernel address */
cmplw 0,r1,r3
mfspr r2, SPRN_SDR1
- li r1, _PAGE_PRESENT | _PAGE_ACCESSED
+ li r1, _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1, _PAGE_PRESENT | _PAGE_ACCESSED
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
lwz r2,0(r2) /* get pmd entry */
@@ -593,10 +595,11 @@ DataStoreTLBMiss:
lis r1, TASK_SIZE@h /* check if kernel address */
cmplw 0,r1,r3
mfspr r2, SPRN_SDR1
- li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED
+ li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_USER
rlwinm r2, r2, 28, 0xfffff000
bgt- 112f
lis r2, (swapper_pg_dir - PAGE_OFFSET)@ha /* if kernel address, use */
+ li r1, _PAGE_RW | _PAGE_DIRTY | _PAGE_PRESENT | _PAGE_ACCESSED
addi r2, r2, (swapper_pg_dir - PAGE_OFFSET)@l /* kernel page table */
112: rlwimi r2,r3,12,20,29 /* insert top 10 bits of address */
lwz r2,0(r2) /* get pmd entry */
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/akebono: Fix unmet dependency errors
From: Randy Dunlap @ 2021-02-01 4:11 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: f.fainelli, yury.norov
In-Reply-To: <20210201012503.940145-1-mpe@ellerman.id.au>
On 1/31/21 5:25 PM, Michael Ellerman wrote:
> The AKEBONO config has various selects under it, including some with
> user-selectable dependencies, which means those dependencies can be
> disabled. This leads to warnings from Kconfig.
>
>
> The problem is that AKEBONO is using select to enable things that are
> not true dependencies, but rather things you probably want enabled in
> an AKEBONO kernel. That is what a defconfig is for.
>
> So drop those selects and instead move those symbols into the
> defconfig. This fixes all the kconfig warnings, and the result of make
> 44x/akebono_defconfig is the same before and after the patch.
>
> Reported-by: Yury Norov <yury.norov@gmail.com>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/configs/44x/akebono_defconfig | 5 +++++
> arch/powerpc/platforms/44x/Kconfig | 7 -------
> 2 files changed, 5 insertions(+), 7 deletions(-)
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.
--
~Randy
^ permalink raw reply
* Re: [PATCH] powerpc/akebono: Fix unmet dependency errors
From: Florian Fainelli @ 2021-02-01 2:17 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: rdunlap, yury.norov
In-Reply-To: <20210201012503.940145-1-mpe@ellerman.id.au>
On 1/31/2021 5:25 PM, Michael Ellerman wrote:
> The AKEBONO config has various selects under it, including some with
> user-selectable dependencies, which means those dependencies can be
> disabled. This leads to warnings from Kconfig.
>
> This can be seen with eg:
>
> $ make allnoconfig
> $ ./scripts/config --file build~/.config -k -e CONFIG_44x -k -e CONFIG_PPC_47x -e CONFIG_AKEBONO
> $ make olddefconfig
>
> WARNING: unmet direct dependencies detected for ATA
> Depends on [n]: HAS_IOMEM [=y] && BLOCK [=n]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> WARNING: unmet direct dependencies detected for NETDEVICES
> Depends on [n]: NET [=n]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> WARNING: unmet direct dependencies detected for ETHERNET
> Depends on [n]: NETDEVICES [=y] && NET [=n]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> WARNING: unmet direct dependencies detected for MMC_SDHCI
> Depends on [n]: MMC [=n] && HAS_DMA [=y]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> WARNING: unmet direct dependencies detected for MMC_SDHCI_PLTFM
> Depends on [n]: MMC [=n] && MMC_SDHCI [=y]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> The problem is that AKEBONO is using select to enable things that are
> not true dependencies, but rather things you probably want enabled in
> an AKEBONO kernel. That is what a defconfig is for.
>
> So drop those selects and instead move those symbols into the
> defconfig. This fixes all the kconfig warnings, and the result of make
> 44x/akebono_defconfig is the same before and after the patch.
>
> Reported-by: Yury Norov <yury.norov@gmail.com>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Much better than my patch, thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH] cxl: Simplify bool conversion
From: Andrew Donnellan @ 2021-02-01 1:44 UTC (permalink / raw)
To: Yang Li, fbarrat; +Cc: gregkh, linuxppc-dev, linux-kernel, arnd
In-Reply-To: <1611908705-98507-1-git-send-email-yang.lee@linux.alibaba.com>
On 29/1/21 7:25 pm, Yang Li wrote:
> Fix the following coccicheck warning:
> ./drivers/misc/cxl/sysfs.c:181:48-53: WARNING: conversion to bool not
> needed here
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Thanks!
> ---
> drivers/misc/cxl/sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
> index d97a243..c173a5e 100644
> --- a/drivers/misc/cxl/sysfs.c
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -178,7 +178,7 @@ static ssize_t perst_reloads_same_image_store(struct device *device,
> if ((rc != 1) || !(val == 1 || val == 0))
> return -EINVAL;
>
> - adapter->perst_same_image = (val == 1 ? true : false);
> + adapter->perst_same_image = (val == 1);
> return count;
> }
>
>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* [PATCH] powerpc/akebono: Fix unmet dependency errors
From: Michael Ellerman @ 2021-02-01 1:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: f.fainelli, rdunlap, yury.norov
The AKEBONO config has various selects under it, including some with
user-selectable dependencies, which means those dependencies can be
disabled. This leads to warnings from Kconfig.
This can be seen with eg:
$ make allnoconfig
$ ./scripts/config --file build~/.config -k -e CONFIG_44x -k -e CONFIG_PPC_47x -e CONFIG_AKEBONO
$ make olddefconfig
WARNING: unmet direct dependencies detected for ATA
Depends on [n]: HAS_IOMEM [=y] && BLOCK [=n]
Selected by [y]:
- AKEBONO [=y] && PPC_47x [=y]
WARNING: unmet direct dependencies detected for NETDEVICES
Depends on [n]: NET [=n]
Selected by [y]:
- AKEBONO [=y] && PPC_47x [=y]
WARNING: unmet direct dependencies detected for ETHERNET
Depends on [n]: NETDEVICES [=y] && NET [=n]
Selected by [y]:
- AKEBONO [=y] && PPC_47x [=y]
WARNING: unmet direct dependencies detected for MMC_SDHCI
Depends on [n]: MMC [=n] && HAS_DMA [=y]
Selected by [y]:
- AKEBONO [=y] && PPC_47x [=y]
WARNING: unmet direct dependencies detected for MMC_SDHCI_PLTFM
Depends on [n]: MMC [=n] && MMC_SDHCI [=y]
Selected by [y]:
- AKEBONO [=y] && PPC_47x [=y]
The problem is that AKEBONO is using select to enable things that are
not true dependencies, but rather things you probably want enabled in
an AKEBONO kernel. That is what a defconfig is for.
So drop those selects and instead move those symbols into the
defconfig. This fixes all the kconfig warnings, and the result of make
44x/akebono_defconfig is the same before and after the patch.
Reported-by: Yury Norov <yury.norov@gmail.com>
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/configs/44x/akebono_defconfig | 5 +++++
arch/powerpc/platforms/44x/Kconfig | 7 -------
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/configs/44x/akebono_defconfig b/arch/powerpc/configs/44x/akebono_defconfig
index 3894ba8f8ffc..ae9b7beefdd6 100644
--- a/arch/powerpc/configs/44x/akebono_defconfig
+++ b/arch/powerpc/configs/44x/akebono_defconfig
@@ -21,6 +21,7 @@ CONFIG_IRQ_ALL_CPUS=y
# CONFIG_COMPACTION is not set
# CONFIG_SUSPEND is not set
CONFIG_NET=y
+CONFIG_NETDEVICES=y
CONFIG_PACKET=y
CONFIG_UNIX=y
CONFIG_INET=y
@@ -41,7 +42,9 @@ CONFIG_BLK_DEV_RAM_SIZE=35000
# CONFIG_SCSI_PROC_FS is not set
CONFIG_BLK_DEV_SD=y
# CONFIG_SCSI_LOWLEVEL is not set
+CONFIG_ATA=y
# CONFIG_SATA_PMP is not set
+CONFIG_SATA_AHCI_PLATFORM=y
# CONFIG_ATA_SFF is not set
# CONFIG_NET_VENDOR_3COM is not set
# CONFIG_NET_VENDOR_ADAPTEC is not set
@@ -98,6 +101,8 @@ CONFIG_USB_OHCI_HCD=y
# CONFIG_USB_OHCI_HCD_PCI is not set
CONFIG_USB_STORAGE=y
CONFIG_MMC=y
+CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_PLTFM=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_M41T80=y
CONFIG_EXT2_FS=y
diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
index 78ac6d67a935..7d41e9264510 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -206,17 +206,10 @@ config AKEBONO
select PPC4xx_HSTA_MSI
select I2C
select I2C_IBM_IIC
- select NETDEVICES
- select ETHERNET
- select NET_VENDOR_IBM
select IBM_EMAC_EMAC4 if IBM_EMAC
select USB if USB_SUPPORT
select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD
- select MMC_SDHCI
- select MMC_SDHCI_PLTFM
- select ATA
- select SATA_AHCI_PLATFORM
help
This option enables support for the IBM Akebono (476gtr) evaluation board
--
2.25.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