* Re: [PATCH v2 4/7] cmdline: Add capability to prepend the command line
From: Will Deacon @ 2021-03-03 18:19 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <e1a498d02d47ec2420b404bd5f3e4a00fc628532.1614705851.git.christophe.leroy@csgroup.eu>
On Tue, Mar 02, 2021 at 05:25:20PM +0000, Christophe Leroy wrote:
> This patchs adds an option of prepend a text to the command
> line instead of appending it.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> include/linux/cmdline.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> index ae3610bb0ee2..144346051e01 100644
> --- a/include/linux/cmdline.h
> +++ b/include/linux/cmdline.h
> @@ -31,7 +31,7 @@ static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_
> }
>
> /*
> - * This function will append a builtin command line to the command
> + * This function will append or prepend a builtin command line to the command
> * line provided by the bootloader. Kconfig options can be used to alter
> * the behavior of this builtin command line.
> * @dest: The destination of the final appended/prepended string.
> @@ -50,6 +50,9 @@ static __always_inline void cmdline_build(char *dest, const char *src, size_t le
> cmdline_strlcat(dest, CONFIG_CMDLINE, length);
> return;
> }
> +
> + if (IS_ENABLED(CONFIG_CMDLINE_PREPEND) && sizeof(CONFIG_CMDLINE) > 1)
> + cmdline_strlcat(dest, CONFIG_CMDLINE " ", length);
Same comment as the other patch: I don't think we need to worry about the
sizeof() here.
Will
^ permalink raw reply
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Will Deacon @ 2021-03-03 18:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <dc6576ac-44ff-7db4-d718-7565b83f50b8@csgroup.eu>
On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote:
> Le 03/03/2021 à 18:46, Will Deacon a écrit :
> > On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:
> > > Le 03/03/2021 à 18:28, Will Deacon a écrit :
> > > > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
> > > > > This code provides architectures with a way to build command line
> > > > > based on what is built in the kernel and what is handed over by the
> > > > > bootloader, based on selected compile-time options.
> > > > >
> > > > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > > > ---
> > > > > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 62 insertions(+)
> > > > > create mode 100644 include/linux/cmdline.h
> > > > >
> > > > > diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> > > > > new file mode 100644
> > > > > index 000000000000..ae3610bb0ee2
> > > > > --- /dev/null
> > > > > +++ b/include/linux/cmdline.h
> > > > > @@ -0,0 +1,62 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +#ifndef _LINUX_CMDLINE_H
> > > > > +#define _LINUX_CMDLINE_H
> > > > > +
> > > > > +static __always_inline size_t cmdline_strlen(const char *s)
> > > > > +{
> > > > > + const char *sc;
> > > > > +
> > > > > + for (sc = s; *sc != '\0'; ++sc)
> > > > > + ; /* nothing */
> > > > > + return sc - s;
> > > > > +}
> > > > > +
> > > > > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
> > > > > +{
> > > > > + size_t dsize = cmdline_strlen(dest);
> > > > > + size_t len = cmdline_strlen(src);
> > > > > + size_t res = dsize + len;
> > > > > +
> > > > > + /* This would be a bug */
> > > > > + if (dsize >= count)
> > > > > + return count;
> > > > > +
> > > > > + dest += dsize;
> > > > > + count -= dsize;
> > > > > + if (len >= count)
> > > > > + len = count - 1;
> > > > > + memcpy(dest, src, len);
> > > > > + dest[len] = 0;
> > > > > + return res;
> > > > > +}
> > > >
> > > > Why are these needed instead of using strlen and strlcat directly?
> > >
> > > Because on powerpc (at least), it will be used in prom_init, it is very
> > > early in the boot and KASAN shadow memory is not set up yet so calling
> > > generic string functions would crash the board.
> >
> > Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
> > you not do something similar? Failing that, I think it would be better to
> > offer the option for an arch to implement cmdline_*, but have then point to
> > the normal library routines by default.
>
> I don't think it is possible to setup an earlier early shadow.
>
> At the point we are in prom_init, the code is not yet relocated at the
> address it was linked for, and it is running with the MMU set by the
> bootloader, I can't imagine being able to setup MMU entries for the early
> shadow KASAN yet without breaking everything.
That's very similar to us; we're not relocated, although we are at least
in control of the MMU (which is using a temporary set of page-tables).
> Is it really worth trying to point to the normal library routines by default
> ? It is really only a few lines of code hence only not many bytes, and
> anyway they are in __init section so they get discarded at the end.
I would prefer to use the normal routines by default and allow architectures
to override them based on their needs, otherwise we'll end up trying to
maintain a "lowest-common-dominator" set of string routines that can be
safely run in whatever different constraints different architectures have.
Will
^ permalink raw reply
* Re: [PATCH v2 0/7] Improve boot command line handling
From: Christophe Leroy @ 2021-03-03 18:07 UTC (permalink / raw)
To: Daniel Walker, Rob Herring
Cc: open list:GENERIC INCLUDE/ASM HEADER FILES, devicetree,
Daniel Gimpelevich, Will Deacon, linux-kernel@vger.kernel.org,
Paul Mackerras, linuxppc-dev
In-Reply-To: <20210303173908.GG109100@zorba>
Le 03/03/2021 à 18:39, Daniel Walker a écrit :
> On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote:
>> +Will D
>>
>> On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote:
>>>
>>> On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:
>>>> The purpose of this series is to improve and enhance the
>>>> handling of kernel boot arguments.
>>>>
>>>> It is first focussed on powerpc but also extends the capability
>>>> for other arches.
>>>>
>>>> This is based on suggestion from Daniel Walker <danielwa@cisco.com>
>>>>
>>>
>>>
>>> I don't see a point in your changes at this time. My changes are much more
>>> mature, and you changes don't really make improvements.
>>
>> Not really a helpful comment. What we merge here will be from whomever
>> is persistent and timely in their efforts. But please, work together
>> on a common solution.
>>
>> This one meets my requirements of moving the kconfig and code out of
>> the arches, supports prepend/append, and is up to date.
>
>
> Maintainers are capable of merging whatever they want to merge. However, I
> wouldn't make hasty choices. The changes I've been submitting have been deployed
> on millions of router instances and are more feature rich.
>
> I believe I worked with you on this change, or something like it,
>
> https://lkml.org/lkml/2019/3/19/970
>
> I don't think Christophe has even addressed this.
I thing I have, see
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3b4291271ce4af4941a771e5af5cbba3c8fa1b2a.1614705851.git.christophe.leroy@csgroup.eu/
If you see something missing in that patch, can you tell me.
> I've converted many
> architectures, and Cisco uses my changes on at least 4 different
> architecture. With products deployed and tested.
As far as we know, only powerpc was converted in the last series you submitted, see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=98106&state=*
>
> I will resubmit my changes as soon as I can.
>
Christophe
^ permalink raw reply
* Re: [PATCH v2 6/7] cmdline: Gives architectures opportunity to use generically defined boot cmdline manipulation
From: Will Deacon @ 2021-03-03 17:57 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <2eb6fad3470256fff5c9f33cd876f344abb1628b.1614705851.git.christophe.leroy@csgroup.eu>
On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:
> Most architectures have similar boot command line manipulation
> options. This patchs adds the definition in init/Kconfig, gated by
> CONFIG_HAVE_CMDLINE that the architectures can select to use them.
>
> In order to use this, a few architectures will have to change their
> CONFIG options:
> - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
> - architectures using CONFIG_CMDLINE_OVERRIDE or
> CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.
>
> Architectures also have to define CONFIG_DEFAULT_CMDLINE.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 22946fe5ded9..a0f2ad9467df 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
> Maximum of each of the number of arguments and environment
> variables passed to init from the kernel command line.
>
> +config HAVE_CMDLINE
> + bool
> +
> +config CMDLINE_BOOL
> + bool "Default bootloader kernel arguments"
> + depends on HAVE_CMDLINE
> + help
> + On some platforms, there is currently no way for the boot loader to
> + pass arguments to the kernel. For these platforms, you can supply
> + some command-line options at build time by entering them here. In
> + most cases you will need to specify the root device here.
Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.
> +config CMDLINE
> + string "Initial kernel command string"
s/Initial/Default
which is then consistent with the rest of the text here.
> + depends on CMDLINE_BOOL
Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.
> + default DEFAULT_CMDLINE
> + help
> + On some platforms, there is currently no way for the boot loader to
> + pass arguments to the kernel. For these platforms, you can supply
> + some command-line options at build time by entering them here. In
> + most cases you will need to specify the root device here.
(same stale text)
> +choice
> + prompt "Kernel command line type" if CMDLINE != ""
> + default CMDLINE_FROM_BOOTLOADER
> + help
> + Selects the way you want to use the default kernel arguments.
How about:
"Determines how the default kernel arguments are combined with any
arguments passed by the bootloader"
> +config CMDLINE_FROM_BOOTLOADER
> + bool "Use bootloader kernel arguments if available"
> + help
> + Uses the command-line options passed by the boot loader. If
> + the boot loader doesn't provide any, the default kernel command
> + string provided in CMDLINE will be used.
> +
> +config CMDLINE_EXTEND
Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.
> + bool "Extend bootloader kernel arguments"
"Append to the bootloader kernel arguments"
> + help
> + The default kernel command string will be appended to the
> + command-line arguments provided during boot.
s/provided during boot/provided by the bootloader/
> +
> +config CMDLINE_PREPEND
> + bool "Prepend bootloader kernel arguments"
"Prepend to the bootloader kernel arguments"
> + help
> + The default kernel command string will be prepend to the
> + command-line arguments provided during boot.
s/prepend/prepended/
s/provided during boot/provided by the bootloader/
> +
> +config CMDLINE_FORCE
> + bool "Always use the default kernel command string"
> + help
> + Always use the default kernel command string, even if the boot
> + loader passes other arguments to the kernel.
> + This is useful if you cannot or don't want to change the
> + command-line options your boot loader passes to the kernel.
I find the "This is useful if ..." sentence really confusing, perhaps just
remove it? I'd then tweak it to be:
"Always use the default kernel command string, ignoring any arguments
provided by the bootloader."
Will
^ permalink raw reply
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Christophe Leroy @ 2021-03-03 17:57 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <20210303174627.GC19713@willie-the-truck>
Le 03/03/2021 à 18:46, Will Deacon a écrit :
> On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:
>> Le 03/03/2021 à 18:28, Will Deacon a écrit :
>>> On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
>>>> This code provides architectures with a way to build command line
>>>> based on what is built in the kernel and what is handed over by the
>>>> bootloader, based on selected compile-time options.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 62 insertions(+)
>>>> create mode 100644 include/linux/cmdline.h
>>>>
>>>> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
>>>> new file mode 100644
>>>> index 000000000000..ae3610bb0ee2
>>>> --- /dev/null
>>>> +++ b/include/linux/cmdline.h
>>>> @@ -0,0 +1,62 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef _LINUX_CMDLINE_H
>>>> +#define _LINUX_CMDLINE_H
>>>> +
>>>> +static __always_inline size_t cmdline_strlen(const char *s)
>>>> +{
>>>> + const char *sc;
>>>> +
>>>> + for (sc = s; *sc != '\0'; ++sc)
>>>> + ; /* nothing */
>>>> + return sc - s;
>>>> +}
>>>> +
>>>> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
>>>> +{
>>>> + size_t dsize = cmdline_strlen(dest);
>>>> + size_t len = cmdline_strlen(src);
>>>> + size_t res = dsize + len;
>>>> +
>>>> + /* This would be a bug */
>>>> + if (dsize >= count)
>>>> + return count;
>>>> +
>>>> + dest += dsize;
>>>> + count -= dsize;
>>>> + if (len >= count)
>>>> + len = count - 1;
>>>> + memcpy(dest, src, len);
>>>> + dest[len] = 0;
>>>> + return res;
>>>> +}
>>>
>>> Why are these needed instead of using strlen and strlcat directly?
>>
>> Because on powerpc (at least), it will be used in prom_init, it is very
>> early in the boot and KASAN shadow memory is not set up yet so calling
>> generic string functions would crash the board.
>
> Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
> you not do something similar? Failing that, I think it would be better to
> offer the option for an arch to implement cmdline_*, but have then point to
> the normal library routines by default.
I don't think it is possible to setup an earlier early shadow.
At the point we are in prom_init, the code is not yet relocated at the address it was linked for,
and it is running with the MMU set by the bootloader, I can't imagine being able to setup MMU
entries for the early shadow KASAN yet without breaking everything.
Is it really worth trying to point to the normal library routines by default ? It is really only a
few lines of code hence only not many bytes, and anyway they are in __init section so they get
discarded at the end.
>
>>>> +/*
>>>> + * This function will append a builtin command line to the command
>>>> + * line provided by the bootloader. Kconfig options can be used to alter
>>>> + * the behavior of this builtin command line.
>>>> + * @dest: The destination of the final appended/prepended string.
>>>> + * @src: The starting string or NULL if there isn't one. Must not equal dest.
>>>> + * @length: the length of dest buffer.
>>>> + */
>>>> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
>>>> +{
>>>> + if (length <= 0)
>>>> + return;
>>>> +
>>>> + dest[0] = 0;
>>>> +
>>>> +#ifdef CONFIG_CMDLINE
>>>> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
>>>> + cmdline_strlcat(dest, CONFIG_CMDLINE, length);
>>>> + return;
>>>> + }
>>>> +#endif
>>>
>>> CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
>>> CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?
>>
>> Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it
>> is feasible. I can change that now.
>>
>>>
>>>> + if (dest != src)
>>>> + cmdline_strlcat(dest, src, length);
>>>> +#ifdef CONFIG_CMDLINE
>>>> + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
>>>> + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
>>>> +#endif
>>>
>>> Likewise, but also I'm not sure why the sizeof() is required.
>>
>> It is to avoid adding a white space at the end of the command line when
>> CONFIG_CMDLINE is empty. But maybe it doesn't matter ?
>
> If CONFIG_CMDLINE is empty, I don't think you can select
> CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters).
Ok I'll simplify that when I re-spin.
Christophe
^ permalink raw reply
* [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm, chip-id property
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
The 'chip_id' field of the XIVE CPU structure is used to choose a
target for a source located on the same chip when possible. This field
is assigned on the PowerNV platform using the "ibm,chip-id" property
on pSeries under KVM when NUMA nodes are defined but it is undefined
under PowerVM. The XIVE source structure has a similar field
'src_chip' which is only assigned on the PowerNV platform.
cpu_to_node() returns a compatible value on all platforms, 0 being the
default node. It will also give us the opportunity to set the affinity
of a source on pSeries when we can localize them.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 595310e056f4..b8e456da28aa 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1335,16 +1335,11 @@ static int xive_prepare_cpu(unsigned int cpu)
xc = per_cpu(xive_cpu, cpu);
if (!xc) {
- struct device_node *np;
-
xc = kzalloc_node(sizeof(struct xive_cpu),
GFP_KERNEL, cpu_to_node(cpu));
if (!xc)
return -ENOMEM;
- np = of_get_cpu_node(cpu, NULL);
- if (np)
- xc->chip_id = of_get_ibm_chip_id(np);
- of_node_put(np);
+ xc->chip_id = cpu_to_node(cpu);
xc->hw_ipi = XIVE_BAD_IRQ;
per_cpu(xive_cpu, cpu) = xc;
--
2.26.2
^ permalink raw reply related
* [PATCH v2 3/8] powerpc/xive: Remove useless check on XIVE_IPI_HW_IRQ
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
The IPI interrupt has its own domain now. Testing the HW interrupt
number is not needed anymore.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index e7783760d278..678680531d26 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1396,13 +1396,12 @@ static void xive_flush_cpu_queue(unsigned int cpu, struct xive_cpu *xc)
struct irq_desc *desc = irq_to_desc(irq);
struct irq_data *d = irq_desc_get_irq_data(desc);
struct xive_irq_data *xd;
- unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
/*
* Ignore anything that isn't a XIVE irq and ignore
* IPIs, so can just be dropped.
*/
- if (d->domain != xive_irq_domain || hw_irq == XIVE_IPI_HW_IRQ)
+ if (d->domain != xive_irq_domain)
continue;
/*
--
2.26.2
^ permalink raw reply related
* [PATCH v2 5/8] powerpc/xive: Drop check on irq_data in xive_core_debug_show()
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: kernel test robot, Greg Kurz, Dan Carpenter,
Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
When looping on IRQ descriptor, irq_data is always valid.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal XIVE state")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 7581cb12bb53..60ebd6f4b31d 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1586,6 +1586,8 @@ static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
u32 target;
u8 prio;
u32 lirq;
+ struct xive_irq_data *xd;
+ u64 val;
rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
if (rc) {
@@ -1596,17 +1598,14 @@ static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
seq_printf(m, "IRQ 0x%08x : target=0x%x prio=%02x lirq=0x%x ",
hw_irq, target, prio, lirq);
- if (d) {
- struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
- u64 val = xive_esb_read(xd, XIVE_ESB_GET);
-
- seq_printf(m, "flags=%c%c%c PQ=%c%c",
- xd->flags & XIVE_IRQ_FLAG_STORE_EOI ? 'S' : ' ',
- xd->flags & XIVE_IRQ_FLAG_LSI ? 'L' : ' ',
- xd->flags & XIVE_IRQ_FLAG_H_INT_ESB ? 'H' : ' ',
- val & XIVE_ESB_VAL_P ? 'P' : '-',
- val & XIVE_ESB_VAL_Q ? 'Q' : '-');
- }
+ xd = irq_data_get_irq_handler_data(d);
+ val = xive_esb_read(xd, XIVE_ESB_GET);
+ seq_printf(m, "flags=%c%c%c PQ=%c%c",
+ xd->flags & XIVE_IRQ_FLAG_STORE_EOI ? 'S' : ' ',
+ xd->flags & XIVE_IRQ_FLAG_LSI ? 'L' : ' ',
+ xd->flags & XIVE_IRQ_FLAG_H_INT_ESB ? 'H' : ' ',
+ val & XIVE_ESB_VAL_P ? 'P' : '-',
+ val & XIVE_ESB_VAL_Q ? 'Q' : '-');
seq_puts(m, "\n");
}
--
2.26.2
^ permalink raw reply related
* [PATCH v2 2/8] powerpc/xive: Introduce an IPI interrupt domain
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
The IPI interrupt is a special case of the XIVE IRQ domain. When
mapping and unmapping the interrupts in the Linux interrupt number
space, the HW interrupt number 0 (XIVE_IPI_HW_IRQ) is checked to
distinguish the IPI interrupt from other interrupts of the system.
Simplify the XIVE interrupt domain by introducing a specific domain
for the IPI.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 51 +++++++++++++------------------
1 file changed, 22 insertions(+), 29 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index b8e456da28aa..e7783760d278 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -63,6 +63,8 @@ static const struct xive_ops *xive_ops;
static struct irq_domain *xive_irq_domain;
#ifdef CONFIG_SMP
+static struct irq_domain *xive_ipi_irq_domain;
+
/* The IPIs all use the same logical irq number */
static u32 xive_ipi_irq;
#endif
@@ -1067,20 +1069,32 @@ static struct irq_chip xive_ipi_chip = {
.irq_unmask = xive_ipi_do_nothing,
};
+/*
+ * IPIs are marked per-cpu. We use separate HW interrupts under the
+ * hood but associated with the same "linux" interrupt
+ */
+static int xive_ipi_irq_domain_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ irq_set_chip_and_handler(virq, &xive_ipi_chip, handle_percpu_irq);
+ return 0;
+}
+
+static const struct irq_domain_ops xive_ipi_irq_domain_ops = {
+ .map = xive_ipi_irq_domain_map,
+};
+
static void __init xive_request_ipi(void)
{
unsigned int virq;
- /*
- * Initialization failed, move on, we might manage to
- * reach the point where we display our errors before
- * the system falls appart
- */
- if (!xive_irq_domain)
+ xive_ipi_irq_domain = irq_domain_add_linear(NULL, 1,
+ &xive_ipi_irq_domain_ops, NULL);
+ if (WARN_ON(xive_ipi_irq_domain == NULL))
return;
/* Initialize it */
- virq = irq_create_mapping(xive_irq_domain, XIVE_IPI_HW_IRQ);
+ virq = irq_create_mapping(xive_ipi_irq_domain, XIVE_IPI_HW_IRQ);
xive_ipi_irq = virq;
WARN_ON(request_irq(virq, xive_muxed_ipi_action,
@@ -1178,19 +1192,6 @@ static int xive_irq_domain_map(struct irq_domain *h, unsigned int virq,
*/
irq_clear_status_flags(virq, IRQ_LEVEL);
-#ifdef CONFIG_SMP
- /* IPIs are special and come up with HW number 0 */
- if (hw == XIVE_IPI_HW_IRQ) {
- /*
- * IPIs are marked per-cpu. We use separate HW interrupts under
- * the hood but associated with the same "linux" interrupt
- */
- irq_set_chip_and_handler(virq, &xive_ipi_chip,
- handle_percpu_irq);
- return 0;
- }
-#endif
-
rc = xive_irq_alloc_data(virq, hw);
if (rc)
return rc;
@@ -1202,15 +1203,7 @@ static int xive_irq_domain_map(struct irq_domain *h, unsigned int virq,
static void xive_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
{
- struct irq_data *data = irq_get_irq_data(virq);
- unsigned int hw_irq;
-
- /* XXX Assign BAD number */
- if (!data)
- return;
- hw_irq = (unsigned int)irqd_to_hwirq(data);
- if (hw_irq != XIVE_IPI_HW_IRQ)
- xive_irq_free_data(virq);
+ xive_irq_free_data(virq);
}
static int xive_irq_domain_xlate(struct irq_domain *h, struct device_node *ct,
--
2.26.2
^ permalink raw reply related
* [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
Now that the IPI interrupt has its own domain, the checks on the HW
interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
check on the domain.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 678680531d26..7581cb12bb53 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file *m, int cpu)
seq_puts(m, "\n");
}
-static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct irq_data *d)
+static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
{
- struct irq_chip *chip = irq_data_get_irq_chip(d);
+ unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
int rc;
u32 target;
u8 prio;
u32 lirq;
- if (!is_xive_irq(chip))
- return;
-
rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
if (rc) {
seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
@@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file *m, void *private)
for_each_irq_desc(i, desc) {
struct irq_data *d = irq_desc_get_irq_data(desc);
- unsigned int hw_irq;
-
- if (!d)
- continue;
-
- hw_irq = (unsigned int)irqd_to_hwirq(d);
- /* IPIs are special (HW number 0) */
- if (hw_irq != XIVE_IPI_HW_IRQ)
- xive_debug_show_irq(m, hw_irq, d);
+ if (d->domain == xive_irq_domain)
+ xive_debug_show_irq(m, d);
}
return 0;
}
--
2.26.2
^ permalink raw reply related
* [PATCH v2 7/8] powerpc/xive: Fix xmon command "dxi"
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: kernel test robot, Greg Kurz, Dan Carpenter,
Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
When under xmon, the "dxi" command dumps the state of the XIVE
interrupts. If an interrupt number is specified, only the state of
the associated XIVE interrupt is dumped. This form of the command
lacks an irq_data parameter which is nevertheless used by
xmon_xive_get_irq_config(), leading to an xmon crash.
Fix that by doing a lookup in the system IRQ mapping to query the IRQ
descriptor data. Invalid interrupt numbers, or not belonging to the
XIVE IRQ domain, OPAL event interrupt number for instance, should be
caught by the previous query done at the firmware level.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 97ef27507793 ("powerpc/xive: Fix xmon support on the PowerNV platform")
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/common.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index f6b7b15bbb3a..8eefd152b947 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -255,17 +255,20 @@ notrace void xmon_xive_do_dump(int cpu)
xmon_printf("\n");
}
+static struct irq_data *xive_get_irq_data(u32 hw_irq)
+{
+ unsigned int irq = irq_find_mapping(xive_irq_domain, hw_irq);
+
+ return irq ? irq_get_irq_data(irq) : NULL;
+}
+
int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
{
- struct irq_chip *chip = irq_data_get_irq_chip(d);
int rc;
u32 target;
u8 prio;
u32 lirq;
- if (!is_xive_irq(chip))
- return -EINVAL;
-
rc = xive_ops->get_irq_config(hw_irq, &target, &prio, &lirq);
if (rc) {
xmon_printf("IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
@@ -275,6 +278,9 @@ int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
xmon_printf("IRQ 0x%08x : target=0x%x prio=%02x lirq=0x%x ",
hw_irq, target, prio, lirq);
+ if (!d)
+ d = xive_get_irq_data(hw_irq);
+
if (d) {
struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
u64 val = xive_esb_read(xd, XIVE_ESB_GET);
--
2.26.2
^ permalink raw reply related
* [PATCH v2 0/8] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
Hello,
ipistorm [*] can be used to benchmark the raw interrupt rate of an
interrupt controller by measuring the number of IPIs a system can
sustain. When applied to the XIVE interrupt controller of POWER9 and
POWER10 systems, a significant drop of the interrupt rate can be
observed when crossing the second node boundary.
This is due to the fact that a single IPI interrupt is used for all
CPUs of the system. The structure is shared and the cache line updates
impact greatly the traffic between nodes and the overall IPI
performance.
As a workaround, the impact can be reduced by deactivating the IRQ
lockup detector ("noirqdebug") which does a lot of accounting in the
Linux IRQ descriptor structure and is responsible for most of the
performance penalty.
As a fix, this proposal allocates an IPI interrupt per node, to be
shared by all CPUs of that node. It solves the scaling issue, the IRQ
lockup detector still has an impact but the XIVE interrupt rate scales
linearly. It also improves the "noirqdebug" case as showed in the
tables below.
* P9 DD2.2 - 2s * 64 threads
"noirqdebug"
Mint/s Mint/s
chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
--------------------------------------------------------------
1 0-15 4.984023 4.875405 4.996536 5.048892
0-31 10.879164 10.544040 10.757632 11.037859
0-47 15.345301 14.688764 14.926520 15.310053
0-63 17.064907 17.066812 17.613416 17.874511
2 0-79 11.768764 21.650749 22.689120 22.566508
0-95 10.616812 26.878789 28.434703 28.320324
0-111 10.151693 31.397803 31.771773 32.388122
0-127 9.948502 33.139336 34.875716 35.224548
* P10 DD1 - 4s (not homogeneous) 352 threads
"noirqdebug"
Mint/s Mint/s
chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
--------------------------------------------------------------
1 0-15 2.409402 2.364108 2.383303 2.395091
0-31 6.028325 6.046075 6.089999 6.073750
0-47 8.655178 8.644531 8.712830 8.724702
0-63 11.629652 11.735953 12.088203 12.055979
0-79 14.392321 14.729959 14.986701 14.973073
0-95 12.604158 13.004034 17.528748 17.568095
2 0-111 9.767753 13.719831 19.968606 20.024218
0-127 6.744566 16.418854 22.898066 22.995110
0-143 6.005699 19.174421 25.425622 25.417541
0-159 5.649719 21.938836 27.952662 28.059603
0-175 5.441410 24.109484 31.133915 31.127996
3 0-191 5.318341 24.405322 33.999221 33.775354
0-207 5.191382 26.449769 36.050161 35.867307
0-223 5.102790 29.356943 39.544135 39.508169
0-239 5.035295 31.933051 42.135075 42.071975
0-255 4.969209 34.477367 44.655395 44.757074
4 0-271 4.907652 35.887016 47.080545 47.318537
0-287 4.839581 38.076137 50.464307 50.636219
0-303 4.786031 40.881319 53.478684 53.310759
0-319 4.743750 43.448424 56.388102 55.973969
0-335 4.709936 45.623532 59.400930 58.926857
0-351 4.681413 45.646151 62.035804 61.830057
[*] https://github.com/antonblanchard/ipistorm
Thanks,
C.
Changes in v2:
- extra simplification on xmon
- fixes on issues reported by the kernel test robot
Cédric Le Goater (8):
powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property
powerpc/xive: Introduce an IPI interrupt domain
powerpc/xive: Remove useless check on XIVE_IPI_HW_IRQ
powerpc/xive: Simplify xive_core_debug_show()
powerpc/xive: Drop check on irq_data in xive_core_debug_show()
powerpc/xive: Simplify the dump of XIVE interrupts under xmon
powerpc/xive: Fix xmon command "dxi"
powerpc/xive: Map one IPI interrupt per node
arch/powerpc/include/asm/xive.h | 1 +
arch/powerpc/sysdev/xive/xive-internal.h | 2 -
arch/powerpc/sysdev/xive/common.c | 163 +++++++++++++----------
arch/powerpc/xmon/xmon.c | 28 +---
4 files changed, 93 insertions(+), 101 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH v2 8/8] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
ipistorm [*] can be used to benchmark the raw interrupt rate of an
interrupt controller by measuring the number of IPIs a system can
sustain. When applied to the XIVE interrupt controller of POWER9 and
POWER10 systems, a significant drop of the interrupt rate can be
observed when crossing the second node boundary.
This is due to the fact that a single IPI interrupt is used for all
CPUs of the system. The structure is shared and the cache line updates
impact greatly the traffic between nodes and the overall IPI
performance.
As a workaround, the impact can be reduced by deactivating the IRQ
lockup detector ("noirqdebug") which does a lot of accounting in the
Linux IRQ descriptor structure and is responsible for most of the
performance penalty.
As a fix, this proposal allocates an IPI interrupt per node, to be
shared by all CPUs of that node. It solves the scaling issue, the IRQ
lockup detector still has an impact but the XIVE interrupt rate scales
linearly. It also improves the "noirqdebug" case as showed in the
tables below.
* P9 DD2.2 - 2s * 64 threads
"noirqdebug"
Mint/s Mint/s
chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
--------------------------------------------------------------
1 0-15 4.984023 4.875405 4.996536 5.048892
0-31 10.879164 10.544040 10.757632 11.037859
0-47 15.345301 14.688764 14.926520 15.310053
0-63 17.064907 17.066812 17.613416 17.874511
2 0-79 11.768764 21.650749 22.689120 22.566508
0-95 10.616812 26.878789 28.434703 28.320324
0-111 10.151693 31.397803 31.771773 32.388122
0-127 9.948502 33.139336 34.875716 35.224548
* P10 DD1 - 4s (not homogeneous) 352 threads
"noirqdebug"
Mint/s Mint/s
chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
--------------------------------------------------------------
1 0-15 2.409402 2.364108 2.383303 2.395091
0-31 6.028325 6.046075 6.089999 6.073750
0-47 8.655178 8.644531 8.712830 8.724702
0-63 11.629652 11.735953 12.088203 12.055979
0-79 14.392321 14.729959 14.986701 14.973073
0-95 12.604158 13.004034 17.528748 17.568095
2 0-111 9.767753 13.719831 19.968606 20.024218
0-127 6.744566 16.418854 22.898066 22.995110
0-143 6.005699 19.174421 25.425622 25.417541
0-159 5.649719 21.938836 27.952662 28.059603
0-175 5.441410 24.109484 31.133915 31.127996
3 0-191 5.318341 24.405322 33.999221 33.775354
0-207 5.191382 26.449769 36.050161 35.867307
0-223 5.102790 29.356943 39.544135 39.508169
0-239 5.035295 31.933051 42.135075 42.071975
0-255 4.969209 34.477367 44.655395 44.757074
4 0-271 4.907652 35.887016 47.080545 47.318537
0-287 4.839581 38.076137 50.464307 50.636219
0-303 4.786031 40.881319 53.478684 53.310759
0-319 4.743750 43.448424 56.388102 55.973969
0-335 4.709936 45.623532 59.400930 58.926857
0-351 4.681413 45.646151 62.035804 61.830057
[*] https://github.com/antonblanchard/ipistorm
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xive/xive-internal.h | 2 --
arch/powerpc/sysdev/xive/common.c | 39 ++++++++++++++++++------
2 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/xive-internal.h b/arch/powerpc/sysdev/xive/xive-internal.h
index 9cf57c722faa..b3a456fdd3a5 100644
--- a/arch/powerpc/sysdev/xive/xive-internal.h
+++ b/arch/powerpc/sysdev/xive/xive-internal.h
@@ -5,8 +5,6 @@
#ifndef __XIVE_INTERNAL_H
#define __XIVE_INTERNAL_H
-#define XIVE_IPI_HW_IRQ 0 /* interrupt source # for IPIs */
-
/*
* A "disabled" interrupt should never fire, to catch problems
* we set its logical number to this
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 8eefd152b947..c27f7bb0494b 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -65,8 +65,16 @@ static struct irq_domain *xive_irq_domain;
#ifdef CONFIG_SMP
static struct irq_domain *xive_ipi_irq_domain;
-/* The IPIs all use the same logical irq number */
-static u32 xive_ipi_irq;
+/* The IPIs use the same logical irq number when on the same chip */
+static struct xive_ipi_desc {
+ unsigned int irq;
+ char name[8]; /* enough bytes to fit IPI-XXX */
+} *xive_ipis;
+
+static unsigned int xive_ipi_cpu_to_irq(unsigned int cpu)
+{
+ return xive_ipis[cpu_to_node(cpu)].irq;
+}
#endif
/* Xive state for each CPU */
@@ -1106,25 +1114,36 @@ static const struct irq_domain_ops xive_ipi_irq_domain_ops = {
static void __init xive_request_ipi(void)
{
- unsigned int virq;
+ unsigned int node;
- xive_ipi_irq_domain = irq_domain_add_linear(NULL, 1,
+ xive_ipi_irq_domain = irq_domain_add_linear(NULL, nr_node_ids,
&xive_ipi_irq_domain_ops, NULL);
if (WARN_ON(xive_ipi_irq_domain == NULL))
return;
- /* Initialize it */
- virq = irq_create_mapping(xive_ipi_irq_domain, XIVE_IPI_HW_IRQ);
- xive_ipi_irq = virq;
+ xive_ipis = kcalloc(nr_node_ids, sizeof(*xive_ipis), GFP_KERNEL | __GFP_NOFAIL);
+ for_each_node(node) {
+ struct xive_ipi_desc *xid = &xive_ipis[node];
+ irq_hw_number_t node_ipi_hwirq = node;
+
+ /*
+ * Map one IPI interrupt per node for all cpus of that node.
+ * Since the HW interrupt number doesn't have any meaning,
+ * simply use the node number.
+ */
+ xid->irq = irq_create_mapping(xive_ipi_irq_domain, node_ipi_hwirq);
+ snprintf(xid->name, sizeof(xid->name), "IPI-%d", node);
- WARN_ON(request_irq(virq, xive_muxed_ipi_action,
- IRQF_PERCPU | IRQF_NO_THREAD, "IPI", NULL));
+ WARN_ON(request_irq(xid->irq, xive_muxed_ipi_action,
+ IRQF_PERCPU | IRQF_NO_THREAD, xid->name, NULL));
+ }
}
static int xive_setup_cpu_ipi(unsigned int cpu)
{
struct xive_cpu *xc;
int rc;
+ unsigned int xive_ipi_irq = xive_ipi_cpu_to_irq(cpu);
pr_debug("Setting up IPI for CPU %d\n", cpu);
@@ -1165,6 +1184,8 @@ static int xive_setup_cpu_ipi(unsigned int cpu)
static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc)
{
+ unsigned int xive_ipi_irq = xive_ipi_cpu_to_irq(cpu);
+
/* Disable the IPI and free the IRQ data */
/* Already cleaned up ? */
--
2.26.2
^ permalink raw reply related
* [PATCH v2 6/8] powerpc/xive: Simplify the dump of XIVE interrupts under xmon
From: Cédric Le Goater @ 2021-03-03 17:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Greg Kurz, Cédric Le Goater
In-Reply-To: <20210303174857.1760393-1-clg@kaod.org>
Move the xmon routine under XIVE subsystem and rework the loop on the
interrupts taking into account the xive_irq_domain to filter out IPIs.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/xive.h | 1 +
arch/powerpc/sysdev/xive/common.c | 14 ++++++++++++++
arch/powerpc/xmon/xmon.c | 28 ++--------------------------
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
index 9a312b975ca8..aa094a8655b0 100644
--- a/arch/powerpc/include/asm/xive.h
+++ b/arch/powerpc/include/asm/xive.h
@@ -102,6 +102,7 @@ void xive_flush_interrupt(void);
/* xmon hook */
void xmon_xive_do_dump(int cpu);
int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d);
+void xmon_xive_get_irq_all(void);
/* APIs used by KVM */
u32 xive_native_default_eq_shift(void);
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 60ebd6f4b31d..f6b7b15bbb3a 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -291,6 +291,20 @@ int xmon_xive_get_irq_config(u32 hw_irq, struct irq_data *d)
return 0;
}
+void xmon_xive_get_irq_all(void)
+{
+ unsigned int i;
+ struct irq_desc *desc;
+
+ for_each_irq_desc(i, desc) {
+ struct irq_data *d = irq_desc_get_irq_data(desc);
+ unsigned int hwirq = (unsigned int)irqd_to_hwirq(d);
+
+ if (d->domain == xive_irq_domain)
+ xmon_xive_get_irq_config(hwirq, d);
+ }
+}
+
#endif /* CONFIG_XMON */
static unsigned int xive_get_irq(void)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3fe37495f63d..80fbf8968f77 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2727,30 +2727,6 @@ static void dump_all_xives(void)
dump_one_xive(cpu);
}
-static void dump_one_xive_irq(u32 num, struct irq_data *d)
-{
- xmon_xive_get_irq_config(num, d);
-}
-
-static void dump_all_xive_irq(void)
-{
- unsigned int i;
- struct irq_desc *desc;
-
- for_each_irq_desc(i, desc) {
- struct irq_data *d = irq_desc_get_irq_data(desc);
- unsigned int hwirq;
-
- if (!d)
- continue;
-
- hwirq = (unsigned int)irqd_to_hwirq(d);
- /* IPIs are special (HW number 0) */
- if (hwirq)
- dump_one_xive_irq(hwirq, d);
- }
-}
-
static void dump_xives(void)
{
unsigned long num;
@@ -2767,9 +2743,9 @@ static void dump_xives(void)
return;
} else if (c == 'i') {
if (scanhex(&num))
- dump_one_xive_irq(num, NULL);
+ xmon_xive_get_irq_config(num, NULL);
else
- dump_all_xive_irq();
+ xmon_xive_get_irq_all();
return;
}
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Will Deacon @ 2021-03-03 17:46 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <a0cfef11-efba-2e5c-6f58-ed63a2c3bfa0@csgroup.eu>
On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:
> Le 03/03/2021 à 18:28, Will Deacon a écrit :
> > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
> > > This code provides architectures with a way to build command line
> > > based on what is built in the kernel and what is handed over by the
> > > bootloader, based on selected compile-time options.
> > >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > ---
> > > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > > create mode 100644 include/linux/cmdline.h
> > >
> > > diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> > > new file mode 100644
> > > index 000000000000..ae3610bb0ee2
> > > --- /dev/null
> > > +++ b/include/linux/cmdline.h
> > > @@ -0,0 +1,62 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _LINUX_CMDLINE_H
> > > +#define _LINUX_CMDLINE_H
> > > +
> > > +static __always_inline size_t cmdline_strlen(const char *s)
> > > +{
> > > + const char *sc;
> > > +
> > > + for (sc = s; *sc != '\0'; ++sc)
> > > + ; /* nothing */
> > > + return sc - s;
> > > +}
> > > +
> > > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
> > > +{
> > > + size_t dsize = cmdline_strlen(dest);
> > > + size_t len = cmdline_strlen(src);
> > > + size_t res = dsize + len;
> > > +
> > > + /* This would be a bug */
> > > + if (dsize >= count)
> > > + return count;
> > > +
> > > + dest += dsize;
> > > + count -= dsize;
> > > + if (len >= count)
> > > + len = count - 1;
> > > + memcpy(dest, src, len);
> > > + dest[len] = 0;
> > > + return res;
> > > +}
> >
> > Why are these needed instead of using strlen and strlcat directly?
>
> Because on powerpc (at least), it will be used in prom_init, it is very
> early in the boot and KASAN shadow memory is not set up yet so calling
> generic string functions would crash the board.
Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.
> > > +/*
> > > + * This function will append a builtin command line to the command
> > > + * line provided by the bootloader. Kconfig options can be used to alter
> > > + * the behavior of this builtin command line.
> > > + * @dest: The destination of the final appended/prepended string.
> > > + * @src: The starting string or NULL if there isn't one. Must not equal dest.
> > > + * @length: the length of dest buffer.
> > > + */
> > > +static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
> > > +{
> > > + if (length <= 0)
> > > + return;
> > > +
> > > + dest[0] = 0;
> > > +
> > > +#ifdef CONFIG_CMDLINE
> > > + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
> > > + cmdline_strlcat(dest, CONFIG_CMDLINE, length);
> > > + return;
> > > + }
> > > +#endif
> >
> > CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
> > CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?
>
> Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it
> is feasible. I can change that now.
>
> >
> > > + if (dest != src)
> > > + cmdline_strlcat(dest, src, length);
> > > +#ifdef CONFIG_CMDLINE
> > > + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
> > > + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
> > > +#endif
> >
> > Likewise, but also I'm not sure why the sizeof() is required.
>
> It is to avoid adding a white space at the end of the command line when
> CONFIG_CMDLINE is empty. But maybe it doesn't matter ?
If CONFIG_CMDLINE is empty, I don't think you can select
CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters).
Will
^ permalink raw reply
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Christophe Leroy @ 2021-03-03 17:38 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <20210303172810.GA19713@willie-the-truck>
Le 03/03/2021 à 18:28, Will Deacon a écrit :
> On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
>> This code provides architectures with a way to build command line
>> based on what is built in the kernel and what is handed over by the
>> bootloader, based on selected compile-time options.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>> create mode 100644 include/linux/cmdline.h
>>
>> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
>> new file mode 100644
>> index 000000000000..ae3610bb0ee2
>> --- /dev/null
>> +++ b/include/linux/cmdline.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _LINUX_CMDLINE_H
>> +#define _LINUX_CMDLINE_H
>> +
>> +static __always_inline size_t cmdline_strlen(const char *s)
>> +{
>> + const char *sc;
>> +
>> + for (sc = s; *sc != '\0'; ++sc)
>> + ; /* nothing */
>> + return sc - s;
>> +}
>> +
>> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
>> +{
>> + size_t dsize = cmdline_strlen(dest);
>> + size_t len = cmdline_strlen(src);
>> + size_t res = dsize + len;
>> +
>> + /* This would be a bug */
>> + if (dsize >= count)
>> + return count;
>> +
>> + dest += dsize;
>> + count -= dsize;
>> + if (len >= count)
>> + len = count - 1;
>> + memcpy(dest, src, len);
>> + dest[len] = 0;
>> + return res;
>> +}
>
> Why are these needed instead of using strlen and strlcat directly?
Because on powerpc (at least), it will be used in prom_init, it is very early in the boot and KASAN
shadow memory is not set up yet so calling generic string functions would crash the board.
>
>> +/*
>> + * This function will append a builtin command line to the command
>> + * line provided by the bootloader. Kconfig options can be used to alter
>> + * the behavior of this builtin command line.
>> + * @dest: The destination of the final appended/prepended string.
>> + * @src: The starting string or NULL if there isn't one. Must not equal dest.
>> + * @length: the length of dest buffer.
>> + */
>> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
>> +{
>> + if (length <= 0)
>> + return;
>> +
>> + dest[0] = 0;
>> +
>> +#ifdef CONFIG_CMDLINE
>> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
>> + cmdline_strlcat(dest, CONFIG_CMDLINE, length);
>> + return;
>> + }
>> +#endif
>
> CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
> CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?
Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it is feasible. I can
change that now.
>
>> + if (dest != src)
>> + cmdline_strlcat(dest, src, length);
>> +#ifdef CONFIG_CMDLINE
>> + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
>> + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
>> +#endif
>
> Likewise, but also I'm not sure why the sizeof() is required.
It is to avoid adding a white space at the end of the command line when CONFIG_CMDLINE is empty. But
maybe it doesn't matter ?
Christophe
^ permalink raw reply
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Will Deacon @ 2021-03-03 17:39 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <d8cf7979ad986de45301b39a757c268d9df19f35.1614705851.git.christophe.leroy@csgroup.eu>
On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
> This code provides architectures with a way to build command line
> based on what is built in the kernel and what is handed over by the
> bootloader, based on selected compile-time options.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 include/linux/cmdline.h
Sorry, spotted a couple of other things...
> +/*
> + * This function will append a builtin command line to the command
> + * line provided by the bootloader. Kconfig options can be used to alter
> + * the behavior of this builtin command line.
> + * @dest: The destination of the final appended/prepended string.
> + * @src: The starting string or NULL if there isn't one. Must not equal dest.
> + * @length: the length of dest buffer.
> + */
> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
> +{
> + if (length <= 0)
> + return;
length is unsigned
> +
> + dest[0] = 0;
> +
> +#ifdef CONFIG_CMDLINE
> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
> + cmdline_strlcat(dest, CONFIG_CMDLINE, length);
> + return;
> + }
> +#endif
> + if (dest != src)
The kernel-doc says that @src "Must not equal dest".
Will
^ permalink raw reply
* Re: [PATCH v2 0/7] Improve boot command line handling
From: Daniel Walker @ 2021-03-03 17:39 UTC (permalink / raw)
To: Rob Herring
Cc: open list:GENERIC INCLUDE/ASM HEADER FILES, devicetree,
Daniel Gimpelevich, Will Deacon, linux-kernel@vger.kernel.org,
Paul Mackerras, linuxppc-dev
In-Reply-To: <CAL_JsqJ7U8QAbJe3zkZiFPJN4PveHz5TZoPk2S8qQWB6cm5e5Q@mail.gmail.com>
On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote:
> +Will D
>
> On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote:
> >
> > On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:
> > > The purpose of this series is to improve and enhance the
> > > handling of kernel boot arguments.
> > >
> > > It is first focussed on powerpc but also extends the capability
> > > for other arches.
> > >
> > > This is based on suggestion from Daniel Walker <danielwa@cisco.com>
> > >
> >
> >
> > I don't see a point in your changes at this time. My changes are much more
> > mature, and you changes don't really make improvements.
>
> Not really a helpful comment. What we merge here will be from whomever
> is persistent and timely in their efforts. But please, work together
> on a common solution.
>
> This one meets my requirements of moving the kconfig and code out of
> the arches, supports prepend/append, and is up to date.
Maintainers are capable of merging whatever they want to merge. However, I
wouldn't make hasty choices. The changes I've been submitting have been deployed
on millions of router instances and are more feature rich.
I believe I worked with you on this change, or something like it,
https://lkml.org/lkml/2019/3/19/970
I don't think Christophe has even addressed this. I've converted many
architectures, and Cisco uses my changes on at least 4 different
architecture. With products deployed and tested.
I will resubmit my changes as soon as I can.
Daniel
^ permalink raw reply
* Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
From: Will Deacon @ 2021-03-03 17:28 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, robh, daniel, devicetree, linux-kernel,
Paul Mackerras, linuxppc-dev, danielwa
In-Reply-To: <d8cf7979ad986de45301b39a757c268d9df19f35.1614705851.git.christophe.leroy@csgroup.eu>
On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:
> This code provides architectures with a way to build command line
> based on what is built in the kernel and what is handed over by the
> bootloader, based on selected compile-time options.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 include/linux/cmdline.h
>
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..ae3610bb0ee2
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_CMDLINE_H
> +#define _LINUX_CMDLINE_H
> +
> +static __always_inline size_t cmdline_strlen(const char *s)
> +{
> + const char *sc;
> +
> + for (sc = s; *sc != '\0'; ++sc)
> + ; /* nothing */
> + return sc - s;
> +}
> +
> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
> +{
> + size_t dsize = cmdline_strlen(dest);
> + size_t len = cmdline_strlen(src);
> + size_t res = dsize + len;
> +
> + /* This would be a bug */
> + if (dsize >= count)
> + return count;
> +
> + dest += dsize;
> + count -= dsize;
> + if (len >= count)
> + len = count - 1;
> + memcpy(dest, src, len);
> + dest[len] = 0;
> + return res;
> +}
Why are these needed instead of using strlen and strlcat directly?
> +/*
> + * This function will append a builtin command line to the command
> + * line provided by the bootloader. Kconfig options can be used to alter
> + * the behavior of this builtin command line.
> + * @dest: The destination of the final appended/prepended string.
> + * @src: The starting string or NULL if there isn't one. Must not equal dest.
> + * @length: the length of dest buffer.
> + */
> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
> +{
> + if (length <= 0)
> + return;
> +
> + dest[0] = 0;
> +
> +#ifdef CONFIG_CMDLINE
> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
> + cmdline_strlcat(dest, CONFIG_CMDLINE, length);
> + return;
> + }
> +#endif
CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?
> + if (dest != src)
> + cmdline_strlcat(dest, src, length);
> +#ifdef CONFIG_CMDLINE
> + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
> + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
> +#endif
Likewise, but also I'm not sure why the sizeof() is required.
Will
^ permalink raw reply
* [PATCH v2] powerpc: Fix save_stack_trace_regs() to have running function as first entry
From: Christophe Leroy @ 2021-03-03 17:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, elver,
rostedt
Cc: linuxppc-dev, linux-kernel, kasan-dev
It seems like other architectures, namely x86 and arm64
at least, include the running function as top entry when saving
stack trace with save_stack_trace_regs().
Functionnalities like KFENCE expect it.
Do the same on powerpc, it allows KFENCE to properly identify the faulting
function as depicted below. Before the patch KFENCE was identifying
finish_task_switch.isra as the faulting function.
[ 14.937370] ==================================================================
[ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
[ 14.948692]
[ 14.956814] Invalid read at 0xdf98800a:
[ 14.960664] test_invalid_access+0x54/0x108
[ 14.964876] finish_task_switch.isra.0+0x54/0x23c
[ 14.969606] kunit_try_run_case+0x5c/0xd0
[ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 14.979079] kthread+0x15c/0x174
[ 14.982342] ret_from_kernel_thread+0x14/0x1c
[ 14.986731]
[ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
[ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
[ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
[ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 15.022043] DAR: df98800a DSISR: 20000000
[ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
[ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
[ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
[ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.051181] Call Trace:
[ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
[ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
[ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
[ 15.085798] Instruction dump:
[ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
[ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
[ 15.104612] ==================================================================
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 35de3b1aa168 ("powerpc: Implement save_stack_trace_regs() to enable kprobe stack tracing")
Cc: stable@vger.kernel.org
Acked-by: Marco Elver <elver@google.com>
---
arch/powerpc/kernel/stacktrace.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index b6440657ef92..a99bd3697286 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -23,6 +23,18 @@
#include <asm/paca.h>
+static bool save_entry(struct stack_trace *trace, unsigned long ip, int savesched)
+{
+ if (savesched || !in_sched_functions(ip)) {
+ if (!trace->skip)
+ trace->entries[trace->nr_entries++] = ip;
+ else
+ trace->skip--;
+ }
+ /* Returns true when the trace is full */
+ return trace->nr_entries >= trace->max_entries;
+}
+
/*
* Save stack-backtrace addresses into a stack_trace buffer.
*/
@@ -39,14 +51,7 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
- if (savesched || !in_sched_functions(ip)) {
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
- }
-
- if (trace->nr_entries >= trace->max_entries)
+ if (save_entry(trace, ip, savesched))
return;
sp = newsp;
@@ -84,6 +89,9 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
void
save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
{
+ if (save_entry(trace, regs->nip, 0))
+ return;
+
save_context_stack(trace, regs->gpr[1], current, 0);
}
EXPORT_SYMBOL_GPL(save_stack_trace_regs);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] ibmvnic: Fix possibly uninitialized old_num_tx_queues variable warning.
From: patchwork-bot+netdevbpf @ 2021-03-03 16:50 UTC (permalink / raw)
To: Michal Suchanek
Cc: netdev, linux-kernel, kuba, ljp, drt, paulus, sukadev,
linuxppc-dev, davem
In-Reply-To: <20210302194747.21704-1-msuchanek@suse.de>
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Tue, 2 Mar 2021 20:47:47 +0100 you wrote:
> GCC 7.5 reports:
> ../drivers/net/ethernet/ibm/ibmvnic.c: In function 'ibmvnic_reset_init':
> ../drivers/net/ethernet/ibm/ibmvnic.c:5373:51: warning: 'old_num_tx_queues' may be used uninitialized in this function [-Wmaybe-uninitialized]
> ../drivers/net/ethernet/ibm/ibmvnic.c:5373:6: warning: 'old_num_rx_queues' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> The variable is initialized only if(reset) and used only if(reset &&
> something) so this is a false positive. However, there is no reason to
> not initialize the variables unconditionally avoiding the warning.
>
> [...]
Here is the summary with links:
- ibmvnic: Fix possibly uninitialized old_num_tx_queues variable warning.
https://git.kernel.org/netdev/net/c/6881b07fdd24
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH next v4 00/15] printk: remove logbuf_lock
From: Petr Mladek @ 2021-03-03 15:34 UTC (permalink / raw)
To: John Ogness
Cc: linux-hyperv, Sergey Senozhatsky, Douglas Anderson, linux-mtd,
Miquel Raynal, K. Y. Srinivasan, Thomas Meyer,
Vignesh Raghavendra, Daniel Thompson, Madhavan Srinivasan,
Stephen Hemminger, Richard Weinberger, Anton Vorontsov,
Jordan Niethe, Anton Ivanov, Wei Li, Haiyang Zhang, Ravi Bangoria,
Kees Cook, Alistair Popple, Jeff Dike, Colin Cross, linux-um,
Wei Liu, Steven Rostedt, Davidlohr Bueso, Nicholas Piggin,
Oleg Nesterov, Thomas Gleixner, Andy Shevchenko, Michael Kelley,
Christophe Leroy, Sumit Garg, Tony Luck, Pavel Tatashin,
linux-kernel, Sergey Senozhatsky, Jason Wessel, kgdb-bugreport,
Paul Mackerras, linuxppc-dev, Mike Rapoport
In-Reply-To: <20210303101528.29901-1-john.ogness@linutronix.de>
On Wed 2021-03-03 11:15:13, John Ogness wrote:
> Hello,
>
> Here is v4 of a series to remove @logbuf_lock, exposing the
> ringbuffer locklessly to both readers and writers. v3 is
> here [0].
The series look ready. I am going to push it into printk/linux.git
the following week unless anyone speaks against it in the meantime.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-03 15:20 UTC (permalink / raw)
To: Christophe Leroy
Cc: Mark Rutland, Catalin Marinas, Will Deacon, LKML, kasan-dev,
Paul Mackerras, linuxppc-dev, linux-arm-kernel
In-Reply-To: <1802be3e-dc1a-52e0-1754-a40f0ea39658@csgroup.eu>
On Wed, Mar 03, 2021 at 03:52PM +0100, Christophe Leroy wrote:
> Le 03/03/2021 à 15:38, Marco Elver a écrit :
> > On Wed, 3 Mar 2021 at 15:09, Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> > >
> > > It seems like all other sane architectures, namely x86 and arm64
> > > at least, include the running function as top entry when saving
> > > stack trace.
> > >
> > > Functionnalities like KFENCE expect it.
> > >
> > > Do the same on powerpc, it allows KFENCE to properly identify the faulting
> > > function as depicted below. Before the patch KFENCE was identifying
> > > finish_task_switch.isra as the faulting function.
> > >
> > > [ 14.937370] ==================================================================
> > > [ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
> > > [ 14.948692]
> > > [ 14.956814] Invalid read at 0xdf98800a:
> > > [ 14.960664] test_invalid_access+0x54/0x108
> > > [ 14.964876] finish_task_switch.isra.0+0x54/0x23c
> > > [ 14.969606] kunit_try_run_case+0x5c/0xd0
> > > [ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
> > > [ 14.979079] kthread+0x15c/0x174
> > > [ 14.982342] ret_from_kernel_thread+0x14/0x1c
> > > [ 14.986731]
> > > [ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
> > > [ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
> > > [ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
> > > [ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
> > > [ 15.022043] DAR: df98800a DSISR: 20000000
> > > [ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
> > > [ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
> > > [ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
> > > [ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
> > > [ 15.051181] Call Trace:
> > > [ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
> > > [ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
> > > [ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
> > > [ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
> > > [ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
> > > [ 15.085798] Instruction dump:
> > > [ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
> > > [ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
> > > [ 15.104612] ==================================================================
> > >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >
> > Acked-by: Marco Elver <elver@google.com>
> >
> > Thank you, I think this looks like the right solution. Just a question below:
> >
> ...
>
> > > @@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
> > >
> > > sp = current_stack_frame();
> > >
> > > - save_context_stack(trace, sp, current, 1);
> > > + save_context_stack(trace, sp, (unsigned long)save_stack_trace, current, 1);
> >
> > This causes ip == save_stack_trace and also below for
> > save_stack_trace_tsk. Does this mean save_stack_trace() is included in
> > the trace? Looking at kernel/stacktrace.c, I think the library wants
> > to exclude itself from the trace, as it does '.skip = skipnr + 1' (and
> > '.skip = skipnr + (current == tsk)' for the _tsk variant).
> >
> > If the arch-helper here is included, should this use _RET_IP_ instead?
> >
>
> Don't really know, I was inspired by arm64 which has:
>
> void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> struct task_struct *task, struct pt_regs *regs)
> {
> struct stackframe frame;
>
> if (regs)
> start_backtrace(&frame, regs->regs[29], regs->pc);
> else if (task == current)
> start_backtrace(&frame,
> (unsigned long)__builtin_frame_address(0),
> (unsigned long)arch_stack_walk);
> else
> start_backtrace(&frame, thread_saved_fp(task),
> thread_saved_pc(task));
>
> walk_stackframe(task, &frame, consume_entry, cookie);
> }
>
> But looking at x86 you may be right, so what should be done really ?
x86:
[ 2.843292] calling stack_trace_save:
[ 2.843705] test_func+0x6c/0x118
[ 2.844184] do_one_initcall+0x58/0x270
[ 2.844618] kernel_init_freeable+0x1da/0x23a
[ 2.845110] kernel_init+0xc/0x166
[ 2.845494] ret_from_fork+0x22/0x30
[ 2.867525] calling stack_trace_save_tsk:
[ 2.868017] test_func+0xa9/0x118
[ 2.868530] do_one_initcall+0x58/0x270
[ 2.869003] kernel_init_freeable+0x1da/0x23a
[ 2.869535] kernel_init+0xc/0x166
[ 2.869957] ret_from_fork+0x22/0x30
arm64:
[ 3.786911] calling stack_trace_save:
[ 3.787147] stack_trace_save+0x50/0x78
[ 3.787443] test_func+0x84/0x13c
[ 3.787738] do_one_initcall+0x5c/0x310
[ 3.788099] kernel_init_freeable+0x214/0x294
[ 3.788363] kernel_init+0x18/0x164
[ 3.788585] ret_from_fork+0x10/0x30
[ 3.803615] calling stack_trace_save_tsk:
[ 3.804266] stack_trace_save_tsk+0x9c/0x100
[ 3.804541] test_func+0xc4/0x13c
[ 3.804803] do_one_initcall+0x5c/0x310
[ 3.805031] kernel_init_freeable+0x214/0x294
[ 3.805284] kernel_init+0x18/0x164
[ 3.805505] ret_from_fork+0x10/0x30
+Cc arm64 folks.
So I think the arm64 version also has a bug, because I think a user of
<linux/stacktrace.h> really doesn't care about the library function
itself. And from reading kernel/stacktrace.c I think it wants to exclude
itself entirely.
It's a shame that <linux/stacktrace.h> isn't better documented, but I'm
pretty sure that including the library functions in the trace is not
useful.
For the ppc version, let's do what x86 does and start with the caller.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Christophe Leroy @ 2021-03-03 14:52 UTC (permalink / raw)
To: Marco Elver; +Cc: LKML, kasan-dev, Paul Mackerras, linuxppc-dev
In-Reply-To: <CANpmjNOvgbUCf0QBs1J-mO0yEPuzcTMm7aS1JpPB-17_LabNHw@mail.gmail.com>
Le 03/03/2021 à 15:38, Marco Elver a écrit :
> On Wed, 3 Mar 2021 at 15:09, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> It seems like all other sane architectures, namely x86 and arm64
>> at least, include the running function as top entry when saving
>> stack trace.
>>
>> Functionnalities like KFENCE expect it.
>>
>> Do the same on powerpc, it allows KFENCE to properly identify the faulting
>> function as depicted below. Before the patch KFENCE was identifying
>> finish_task_switch.isra as the faulting function.
>>
>> [ 14.937370] ==================================================================
>> [ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
>> [ 14.948692]
>> [ 14.956814] Invalid read at 0xdf98800a:
>> [ 14.960664] test_invalid_access+0x54/0x108
>> [ 14.964876] finish_task_switch.isra.0+0x54/0x23c
>> [ 14.969606] kunit_try_run_case+0x5c/0xd0
>> [ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
>> [ 14.979079] kthread+0x15c/0x174
>> [ 14.982342] ret_from_kernel_thread+0x14/0x1c
>> [ 14.986731]
>> [ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
>> [ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
>> [ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
>> [ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
>> [ 15.022043] DAR: df98800a DSISR: 20000000
>> [ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
>> [ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
>> [ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
>> [ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
>> [ 15.051181] Call Trace:
>> [ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
>> [ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
>> [ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
>> [ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
>> [ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
>> [ 15.085798] Instruction dump:
>> [ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
>> [ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
>> [ 15.104612] ==================================================================
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> Acked-by: Marco Elver <elver@google.com>
>
> Thank you, I think this looks like the right solution. Just a question below:
>
...
>> @@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
>>
>> sp = current_stack_frame();
>>
>> - save_context_stack(trace, sp, current, 1);
>> + save_context_stack(trace, sp, (unsigned long)save_stack_trace, current, 1);
>
> This causes ip == save_stack_trace and also below for
> save_stack_trace_tsk. Does this mean save_stack_trace() is included in
> the trace? Looking at kernel/stacktrace.c, I think the library wants
> to exclude itself from the trace, as it does '.skip = skipnr + 1' (and
> '.skip = skipnr + (current == tsk)' for the _tsk variant).
>
> If the arch-helper here is included, should this use _RET_IP_ instead?
>
Don't really know, I was inspired by arm64 which has:
void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
struct task_struct *task, struct pt_regs *regs)
{
struct stackframe frame;
if (regs)
start_backtrace(&frame, regs->regs[29], regs->pc);
else if (task == current)
start_backtrace(&frame,
(unsigned long)__builtin_frame_address(0),
(unsigned long)arch_stack_walk);
else
start_backtrace(&frame, thread_saved_fp(task),
thread_saved_pc(task));
walk_stackframe(task, &frame, consume_entry, cookie);
}
But looking at x86 you may be right, so what should be done really ?
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-03 14:38 UTC (permalink / raw)
To: Christophe Leroy; +Cc: LKML, kasan-dev, Paul Mackerras, linuxppc-dev
In-Reply-To: <e2e8728c4c4553bbac75a64b148e402183699c0c.1614780567.git.christophe.leroy@csgroup.eu>
On Wed, 3 Mar 2021 at 15:09, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> It seems like all other sane architectures, namely x86 and arm64
> at least, include the running function as top entry when saving
> stack trace.
>
> Functionnalities like KFENCE expect it.
>
> Do the same on powerpc, it allows KFENCE to properly identify the faulting
> function as depicted below. Before the patch KFENCE was identifying
> finish_task_switch.isra as the faulting function.
>
> [ 14.937370] ==================================================================
> [ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
> [ 14.948692]
> [ 14.956814] Invalid read at 0xdf98800a:
> [ 14.960664] test_invalid_access+0x54/0x108
> [ 14.964876] finish_task_switch.isra.0+0x54/0x23c
> [ 14.969606] kunit_try_run_case+0x5c/0xd0
> [ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
> [ 14.979079] kthread+0x15c/0x174
> [ 14.982342] ret_from_kernel_thread+0x14/0x1c
> [ 14.986731]
> [ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
> [ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
> [ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
> [ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
> [ 15.022043] DAR: df98800a DSISR: 20000000
> [ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
> [ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
> [ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
> [ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
> [ 15.051181] Call Trace:
> [ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
> [ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
> [ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
> [ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
> [ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
> [ 15.085798] Instruction dump:
> [ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
> [ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
> [ 15.104612] ==================================================================
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Marco Elver <elver@google.com>
Thank you, I think this looks like the right solution. Just a question below:
> ---
> arch/powerpc/kernel/stacktrace.c | 42 +++++++++++++++++++++-----------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index b6440657ef92..67c2b8488035 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -22,16 +22,32 @@
> #include <asm/kprobes.h>
>
> #include <asm/paca.h>
> +#include <asm/switch_to.h>
>
> /*
> * Save stack-backtrace addresses into a stack_trace buffer.
> */
> +static void save_entry(struct stack_trace *trace, unsigned long ip, int savesched)
> +{
> + if (savesched || !in_sched_functions(ip)) {
> + if (!trace->skip)
> + trace->entries[trace->nr_entries++] = ip;
> + else
> + trace->skip--;
> + }
> +}
> +
> static void save_context_stack(struct stack_trace *trace, unsigned long sp,
> - struct task_struct *tsk, int savesched)
> + unsigned long ip, struct task_struct *tsk, int savesched)
> {
> + save_entry(trace, ip, savesched);
> +
> + if (trace->nr_entries >= trace->max_entries)
> + return;
> +
> for (;;) {
> unsigned long *stack = (unsigned long *) sp;
> - unsigned long newsp, ip;
> + unsigned long newsp;
>
> if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
> return;
> @@ -39,12 +55,7 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
> newsp = stack[0];
> ip = stack[STACK_FRAME_LR_SAVE];
>
> - if (savesched || !in_sched_functions(ip)) {
> - if (!trace->skip)
> - trace->entries[trace->nr_entries++] = ip;
> - else
> - trace->skip--;
> - }
> + save_entry(trace, ip, savesched);
>
> if (trace->nr_entries >= trace->max_entries)
> return;
> @@ -59,23 +70,26 @@ void save_stack_trace(struct stack_trace *trace)
>
> sp = current_stack_frame();
>
> - save_context_stack(trace, sp, current, 1);
> + save_context_stack(trace, sp, (unsigned long)save_stack_trace, current, 1);
This causes ip == save_stack_trace and also below for
save_stack_trace_tsk. Does this mean save_stack_trace() is included in
the trace? Looking at kernel/stacktrace.c, I think the library wants
to exclude itself from the trace, as it does '.skip = skipnr + 1' (and
'.skip = skipnr + (current == tsk)' for the _tsk variant).
If the arch-helper here is included, should this use _RET_IP_ instead?
Thanks,
-- Marco
> }
> EXPORT_SYMBOL_GPL(save_stack_trace);
>
> void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> {
> - unsigned long sp;
> + unsigned long sp, ip;
>
> if (!try_get_task_stack(tsk))
> return;
>
> - if (tsk == current)
> + if (tsk == current) {
> + ip = (unsigned long)save_stack_trace_tsk;
> sp = current_stack_frame();
> - else
> + } else {
> + ip = (unsigned long)_switch;
> sp = tsk->thread.ksp;
> + }
>
> - save_context_stack(trace, sp, tsk, 0);
> + save_context_stack(trace, sp, ip, tsk, 0);
>
> put_task_stack(tsk);
> }
> @@ -84,7 +98,7 @@ EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
> void
> save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
> {
> - save_context_stack(trace, regs->gpr[1], current, 0);
> + save_context_stack(trace, regs->gpr[1], regs->nip, current, 0);
> }
> EXPORT_SYMBOL_GPL(save_stack_trace_regs);
>
> --
> 2.25.0
>
^ permalink raw reply
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