* Re: [PATCH 2/5] CMDLINE: drivers: of: ifdef out cmdline section
From: Daniel Walker @ 2021-03-04 20:48 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Ruslan Ruslichenko, Daniel Gimpelevich, Frank Rowand,
linuxppc-dev, X86 ML, open list:MIPS,
linux-kernel@vger.kernel.org, xe-linux-external, Andrew Morton,
Will Deacon
In-Reply-To: <CAL_JsqKnAMp0bkXzU-B8b8xx5fPC1R1NdOBn9Kpk=SONJL5paQ@mail.gmail.com>
On Thu, Mar 04, 2021 at 08:32:37AM -0600, Rob Herring wrote:
> On Wed, Mar 3, 2021 at 10:48 PM Daniel Walker <danielwa@cisco.com> wrote:
> >
> > It looks like there's some seepage of cmdline stuff into
> > the generic device tree code. This conflicts with the
> > generic cmdline implementation so I remove it in the case
> > when that's enabled.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> > drivers/of/fdt.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index feb0f2d67fc5..cfe4f8d3c9f5 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> > #include <linux/serial_core.h>
> > #include <linux/sysfs.h>
> > #include <linux/random.h>
> > +#include <linux/cmdline.h>
> >
> > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > #include <asm/page.h>
> > @@ -1048,8 +1049,18 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> >
> > early_init_dt_check_for_initrd(node);
> >
> > +#ifdef CONFIG_GENERIC_CMDLINE
>
> What I like about Christophe's version is it removes the old DT
> implementation. Who's going to convert the rest of the DT based
> arches? That's arm, arm64, hexagon, microblaze, nios2, openrisc,
> riscv, sh, and xtensa. Either separate the common code from the config
> like Christophe's version or these all need converting. Though it's
> fine to hash out patch 1 with a couple of arches first.
I'm limited in what I can test, so I can't know for sure that I have something
which works on those architectures. Even powerpc 64 is part of this series but
I can't really test it at this time. Also Cisco's needs out strip the
implementation of extend or override.
I have un-tested conversions for arm32, arm64, c6x, microblaze, nios2, and
openrisc. These could go into -next and we can see who complains. The
implementation on these architectures isn't all uniform.
> > /* Retrieve command line */
> > p = of_get_flat_dt_prop(node, "bootargs", &l);
>
> This needs to be outside the ifdef.
Ok ..
Daniel
^ permalink raw reply
* Re: [PATCH 1/5] CMDLINE: add generic builtin command line
From: Daniel Walker @ 2021-03-04 21:20 UTC (permalink / raw)
To: Christophe Leroy
Cc: ob Herring, Ruslan Bilovol, Daniel Gimpelevich, linuxppc-dev, x86,
linux-mips, linux-kernel, xe-linux-external, Andrew Morton,
Will Deacon
In-Reply-To: <da33aa9e-ffd8-b012-0f2d-c9ad05f32b8f@csgroup.eu>
On Thu, Mar 04, 2021 at 08:00:49AM +0100, Christophe Leroy wrote:
>
>
> Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> > This code allows architectures to use a generic builtin command line.
> > The state of the builtin command line options across architecture is
> > diverse. On x86 and mips they have pretty much the same code and the
> > code prepends the builtin command line onto the boot loader provided
> > one. On powerpc there is only a builtin override and nothing else.
>
> This is not exact. powerpc has:
> CONFIG_FROM_BOOTLOADER
> CONFIG_EXTEND
> CONFIG_FORCE
I don't currently have ppc64 to test on, but CONFIG_FROM_BOOTLOADER should likely
stay, but the other two can come from the generic code.
> >
> > The code in this commit unifies the code into a generic
> > header file under the CONFIG_GENERIC_CMDLINE option. When this
> > option is enabled the architecture can call the cmdline_add_builtin()
> > to add the builtin command line.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> > include/linux/cmdline.h | 75 +++++++++++++++++++++++++++++++++++++++++
> > init/Kconfig | 68 +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 143 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..f44011d1a9ee
> > --- /dev/null
> > +++ b/include/linux/cmdline.h
> > @@ -0,0 +1,75 @@
>
> Missing the SPDX Licence Identifier
>
> > +#ifndef _LINUX_CMDLINE_H
> > +#define _LINUX_CMDLINE_H
> > +
> > +/*
> > + *
> > + * Copyright (C) 2006,2021. Cisco Systems, Inc.
> > + *
> > + * Generic Append/Prepend cmdline support.
> > + */
> > +
> > +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
>
> I think it would be better if we can avoid the CONFIG_CMDLINE_BOOL.
> By making the CMDLINEs default to "" at all time, I think we can about that BOOL.
Wouldn't it be annoying if you have to deleted all the characters from two text
boxes vs. just disabling a single option ? What if you leave a space
accidentally , woops.
> > +
> > +#ifndef CONFIG_CMDLINE_OVERRIDE
> > +/*
> > + * This function will append or prepend a builtin command line to the command
>
> As far as I understand, it doesn't "append _or_ prepend" but it does "append _and_ prepend"
I think the end results is accurately , no need to get pedantic.
> > + * 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.
> > + * @tmp: temporary space used for prepending
> > + * @length: the maximum length of the strings above.
>
> Missing some parameters here, but I think we should avoid those 'strlcpy'
> and 'strlcat', see later comment.
>
> > + */
> > +static inline void
> > +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
> > + size_t (*strlcpy)(char *dest, const char *src, size_t size),
> > + size_t (*strlcat)(char *dest, const char *src, size_t count)
>
> Don't use names that overide names of existing functions.
>
> 'count' is __kernel_size_t not size_t
It's type checking all the parameters at compile time, it doesn't complain about
this that I've seen.
> > + )
> > +{
> > + if (src != dest && src != NULL) {
> > + strlcpy(dest, " ", length);
>
> Why do you need a space up front in that case ? Why not just copy the source to the destination ?
There may not be a space between them, it doesn't cost anything to have one.
> > + strlcat(dest, src, length);
> > + }
> > +
> > + if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> > + strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
> > +
> > + if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> > + strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
> > + strlcat(tmp, dest, length);
> > + strlcpy(dest, tmp, length);
>
> Could we use memmove(), or implement strmove() and avoid the temporary buffer at all ?
I don't really want to make drastic alteration like this, unless there is a
better reason for it. Most of this hasn't change inside Cisco's tree for almost a decade.
> > + }
> > +}
> > +
> > +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) \
>
> It is misleading to call parameters 'strlcpy' or 'strlcat', it hides that they are overriden.
I can change the names, it's not a big deal.
> > +{ \
> > + if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { \
> > + static label char cmdline_tmp_space[length]; \
>
> Let the architecture define the temporary space when using the custom
> variant instead of just asking the architecture to provide the name of the
> section to use. powerpc already have prom_scratch for that.
How would it use this space exactly ? Is it large enough ? How is it managed?
> > + __cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); \
> > + } else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { \
> > + __cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); \
> > + } \
>
> Ah, so if I understand correctly, the user can set both
> CONFIG_CMDLINE_PREPEND and CONFIG_CMDLINE_APPEND but one of them is silently
> ignored.
Nothing should be ignored. Either one set gets you into the function, just one
has to create a variable.
> Then I think we should just offer the user to set one, name it
> CONFIG_CMDLINE then ask him to choose between FORCE, APPEND or PREPEND.
No, this doesn't work for Cisco. We need to functionality of this solution,
nothing less..
> > +}
> > +#define cmdline_add_builtin(dest, src, length) \
> > + cmdline_add_builtin_custom(dest, src, length, __initdata, &strlcpy, &strlcat)
> > +#else
> > +#define cmdline_add_builtin(dest, src, length) \
> > +{ \
> > + strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND, \
> > + length); \
> > +}
> > +#endif /* !CONFIG_CMDLINE_OVERRIDE */
> > +
> > +#else
> > +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
> > + if (src != NULL) \
> > + strlcpy(dest, src, length); \
> > +}
> > +
> > +#define cmdline_add_builtin(dest, src, length) { \
> > + cmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); \
> > +}
> > +#endif /* CONFIG_GENERIC_CMDLINE */
>
> I'd rather avoid all those macros and use static inline functions instead.
The last two in the off case might be able to be converted.
> For the strlcpy() and strlcat(), use another name, for instance
> cmdline_strlcpy and cmdline_strlcat. Then at the begining of the file,
> define them as strlcpy ad strlcat unless they are already defined to
> something else (by the architecture before including cmdline.h).
Your duplicating your comments.
> > +
> > +
> > +#endif /* _LINUX_CMDLINE_H */
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 29ad68325028..28363ab07cd4 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2032,6 +2032,74 @@ config PROFILING
> > config TRACEPOINTS
> > bool
> > +config GENERIC_CMDLINE
> > + bool
> > +
> > +if GENERIC_CMDLINE
> > +
> > +config CMDLINE_BOOL
> > + bool "Built-in kernel command line"
>
> We don't need the CMDLINE_BOOL, just have CMDLINE always "" by default.
I think it's more usable as explained above.
> > + help
> > + Allow for specifying boot arguments to the kernel at
> > + build time. On some systems (e.g. embedded ones), it is
> > + necessary or convenient to provide some or all of the
> > + kernel boot arguments with the kernel itself (that is,
> > + to not rely on the boot loader to provide them.)
> > +
> > + To compile command line arguments into the kernel,
> > + set this option to 'Y', then fill in the
> > + the boot arguments in CONFIG_CMDLINE.
> > +
> > + Systems with fully functional boot loaders (i.e. non-embedded)
> > + should leave this option set to 'N'.
> > +
> > +config CMDLINE_APPEND
>
> As far as I understand, the generic code will only take CMDLINE_APPEND into
> account if CMDLINE_PREPEND doesn't exist, otherwise it will silently ignore
> it.
No, that's not how that works.
> Only offer one string: CONFIG_CMDLINE, and make the use choose between APPEND, EXTEND or OVERRIDE
No. That's not how this works.
> > + string "Built-in kernel command string append"
> > + depends on CMDLINE_BOOL
> > + default ""
> > + help
> > + Enter arguments here that should be compiled into the kernel
> > + image and used at boot time. If the boot loader provides a
> > + command line at boot time, this string is appended to it to
> > + form the full kernel command line, when the system boots.
> > +
> > + However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> > + change this behavior.
> > +
> > + In most cases, the command line (whether built-in or provided
> > + by the boot loader) should specify the device for the root
> > + file system.
> > +
> > +config CMDLINE_PREPEND
> > + string "Built-in kernel command string prepend"
> > + depends on CMDLINE_BOOL
> > + default ""
> > + help
> > + Enter arguments here that should be compiled into the kernel
> > + image and used at boot time. If the boot loader provides a
> > + command line at boot time, this string is prepended to it to
> > + form the full kernel command line, when the system boots.
> > +
> > + However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> > + change this behavior.
> > +
> > + In most cases, the command line (whether built-in or provided
> > + by the boot loader) should specify the device for the root
> > + file system.
> > +
> > +config CMDLINE_OVERRIDE
> > + bool "Built-in command line overrides boot loader arguments"
> > + depends on CMDLINE_BOOL
> > + help
> > + Set this option to 'Y' to have the kernel ignore the boot loader
> > + command line, and use ONLY the built-in command line. In this case
> > + append and prepend strings are concatenated to form the full
> > + command line.
> > +
> > + This is used to work around broken boot loaders. This should
> > + be set to 'N' under normal conditions.
> > +endif
> > +
> > endmenu # General setup
> > source "arch/Kconfig"
> >
>
> Christophe
Most of your comments are the kind of things this code went thru on it's first
implementation, and were discarded for a reason during usage and testing.
Daniel
^ permalink raw reply
* Re: [PATCH 2/5] CMDLINE: drivers: of: ifdef out cmdline section
From: Daniel Walker @ 2021-03-04 21:24 UTC (permalink / raw)
To: Christophe Leroy
Cc: ob Herring, Ruslan Ruslichenko, Daniel Gimpelevich, Frank Rowand,
devicetree, linuxppc-dev, x86, linux-mips, linux-kernel,
Rob Herring, xe-linux-external, Andrew Morton, Will Deacon
In-Reply-To: <2b0081aa-52af-a4ab-7481-6e125bd103d6@csgroup.eu>
On Thu, Mar 04, 2021 at 08:09:52AM +0100, Christophe Leroy wrote:
>
>
> Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> > It looks like there's some seepage of cmdline stuff into
> > the generic device tree code. This conflicts with the
> > generic cmdline implementation so I remove it in the case
> > when that's enabled.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> > drivers/of/fdt.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index feb0f2d67fc5..cfe4f8d3c9f5 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> > #include <linux/serial_core.h>
> > #include <linux/sysfs.h>
> > #include <linux/random.h>
> > +#include <linux/cmdline.h>
> > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > #include <asm/page.h>
> > @@ -1048,8 +1049,18 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> > early_init_dt_check_for_initrd(node);
> > +#ifdef CONFIG_GENERIC_CMDLINE
> > /* Retrieve command line */
> > p = of_get_flat_dt_prop(node, "bootargs", &l);
> > +
> > + /*
> > + * The builtin command line will be added here, or it can override
> > + * with the DT bootargs.
> > + */
> > + cmdline_add_builtin(data,
> > + ((p != NULL && l > 0) ? p : NULL), /* This is sanity checking */
>
> Can we do more simple ? If p is NULL, p is already NULL, so (l > 0 ? p : NULL) should be enough.
I believe Rob gave me this line. Maybe he can comment on it.
Daniel
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Segher Boessenkool @ 2021-03-04 21:54 UTC (permalink / raw)
To: Mark Rutland
Cc: Marco Elver, Catalin Marinas, linuxppc-dev, LKML, kasan-dev,
broonie, Paul Mackerras, Will Deacon, linux-arm-kernel
In-Reply-To: <20210304145730.GC54534@C02TD0UTHF1T.local>
Hi!
On Thu, Mar 04, 2021 at 02:57:30PM +0000, Mark Rutland wrote:
> It looks like GCC is happy to give us the function-entry-time FP if we use
> __builtin_frame_address(1),
From the GCC manual:
Calling this function with a nonzero argument can have
unpredictable effects, including crashing the calling program. As
a result, calls that are considered unsafe are diagnosed when the
'-Wframe-address' option is in effect. Such calls should only be
made in debugging situations.
It *does* warn (the warning is in -Wall btw), on both powerpc and
aarch64. Furthermore, using this builtin causes lousy code (it forces
the use of a frame pointer, which we normally try very hard to optimise
away, for good reason).
And, that warning is not an idle warning. Non-zero arguments to
__builtin_frame_address can crash the program. It won't on simpler
functions, but there is no real definition of what a simpler function
*is*. It is meant for debugging, not for production use (this is also
why no one has bothered to make it faster).
On Power it should work, but on pretty much any other arch it won't.
> Unless we can get some strong guarantees from compiler folk such that we
> can guarantee a specific function acts boundary for unwinding (and
> doesn't itself get split, etc), the only reliable way I can think to
> solve this requires an assembly trampoline. Whatever we do is liable to
> need some invasive rework.
You cannot get such a guarantee, other than not letting the compiler
see into the routine at all, like with assembler code (not inline asm,
real assembler code).
The real way forward is to bite the bullet and to no longer pretend you
can do a full backtrace from just the stack contents. You cannot.
Segher
^ permalink raw reply
* Re: [PATCH 2/5] CMDLINE: drivers: of: ifdef out cmdline section
From: Rob Herring @ 2021-03-04 22:29 UTC (permalink / raw)
To: Daniel Walker
Cc: devicetree, Ruslan Ruslichenko, Daniel Gimpelevich, Frank Rowand,
linuxppc-dev, X86 ML, open list:MIPS,
linux-kernel@vger.kernel.org, xe-linux-external, Andrew Morton,
Will Deacon
In-Reply-To: <20210304212448.GK109100@zorba>
On Thu, Mar 4, 2021 at 3:24 PM Daniel Walker <danielwa@cisco.com> wrote:
>
> On Thu, Mar 04, 2021 at 08:09:52AM +0100, Christophe Leroy wrote:
> >
> >
> > Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> > > It looks like there's some seepage of cmdline stuff into
> > > the generic device tree code. This conflicts with the
> > > generic cmdline implementation so I remove it in the case
> > > when that's enabled.
> > >
> > > Cc: xe-linux-external@cisco.com
> > > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > > ---
> > > drivers/of/fdt.c | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index feb0f2d67fc5..cfe4f8d3c9f5 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/serial_core.h>
> > > #include <linux/sysfs.h>
> > > #include <linux/random.h>
> > > +#include <linux/cmdline.h>
> > > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > > #include <asm/page.h>
> > > @@ -1048,8 +1049,18 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> > > early_init_dt_check_for_initrd(node);
> > > +#ifdef CONFIG_GENERIC_CMDLINE
> > > /* Retrieve command line */
> > > p = of_get_flat_dt_prop(node, "bootargs", &l);
> > > +
> > > + /*
> > > + * The builtin command line will be added here, or it can override
> > > + * with the DT bootargs.
> > > + */
> > > + cmdline_add_builtin(data,
> > > + ((p != NULL && l > 0) ? p : NULL), /* This is sanity checking */
> >
> > Can we do more simple ? If p is NULL, p is already NULL, so (l > 0 ? p : NULL) should be enough.
>
>
> I believe Rob gave me this line. Maybe he can comment on it.
It's an obvious improvement and LGTM.
^ permalink raw reply
* Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
From: Rob Clark @ 2021-03-04 23:11 UTC (permalink / raw)
To: Robin Murphy
Cc: freedreno, Sai Prakash Ranjan, kvm, Joerg Roedel, linuxppc-dev,
dri-devel, Li Yang,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
netdev, David Woodhouse, linux-arm-msm, virtualization,
Will Deacon, Christoph Hellwig,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <d567ad5c-5f89-effa-7260-88c6d86b4695@arm.com>
On Thu, Mar 4, 2021 at 7:48 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-03-01 08:42, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Moreso than the previous patch, where the feature is at least relatively
> generic (note that there's a bunch of in-flight development around
> DOMAIN_ATTR_NESTING), I'm really not convinced that it's beneficial to
> bloat the generic iommu_ops structure with private driver-specific
> interfaces. The attribute interface is a great compromise for these
> kinds of things, and you can easily add type-checked wrappers around it
> for external callers (maybe even make the actual attributes internal
> between the IOMMU core and drivers) if that's your concern.
I suppose if this is *just* for the GPU we could move it into adreno_smmu_priv..
But one thing I'm not sure about is whether
IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices
*should* be using as well, but just haven't gotten around to yet.
BR,
-R
> Robin.
>
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 40 +++++++------------------
> > drivers/iommu/iommu.c | 9 ++++++
> > include/linux/iommu.h | 9 +++++-
> > 4 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 0f184c3dd9d9ec..78d98ab2ee3a68 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -191,7 +191,7 @@ void adreno_set_llc_attributes(struct iommu_domain *iommu)
> > struct io_pgtable_domain_attr pgtbl_cfg;
> >
> > pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
> > - iommu_domain_set_attr(iommu, DOMAIN_ATTR_IO_PGTABLE_CFG, &pgtbl_cfg);
> > + iommu_domain_set_pgtable_attr(iommu, &pgtbl_cfg);
> > }
> >
> > struct msm_gem_address_space *
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 2e17d990d04481..2858999c86dfd1 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -1515,40 +1515,22 @@ static int arm_smmu_domain_enable_nesting(struct iommu_domain *domain)
> > return ret;
> > }
> >
> > -static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> > - enum iommu_attr attr, void *data)
> > +static int arm_smmu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg)
> > {
> > - int ret = 0;
> > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > + int ret = -EPERM;
> >
> > - mutex_lock(&smmu_domain->init_mutex);
> > -
> > - switch(domain->type) {
> > - case IOMMU_DOMAIN_UNMANAGED:
> > - switch (attr) {
> > - case DOMAIN_ATTR_IO_PGTABLE_CFG: {
> > - struct io_pgtable_domain_attr *pgtbl_cfg = data;
> > -
> > - if (smmu_domain->smmu) {
> > - ret = -EPERM;
> > - goto out_unlock;
> > - }
> > + if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> > + return -EINVAL;
> >
> > - smmu_domain->pgtbl_cfg = *pgtbl_cfg;
> > - break;
> > - }
> > - default:
> > - ret = -ENODEV;
> > - }
> > - break;
> > - case IOMMU_DOMAIN_DMA:
> > - ret = -ENODEV;
> > - break;
> > - default:
> > - ret = -EINVAL;
> > + mutex_lock(&smmu_domain->init_mutex);
> > + if (!smmu_domain->smmu) {
> > + smmu_domain->pgtbl_cfg = *pgtbl_cfg;
> > + ret = 0;
> > }
> > -out_unlock:
> > mutex_unlock(&smmu_domain->init_mutex);
> > +
> > return ret;
> > }
> >
> > @@ -1609,7 +1591,7 @@ static struct iommu_ops arm_smmu_ops = {
> > .device_group = arm_smmu_device_group,
> > .dma_use_flush_queue = arm_smmu_dma_use_flush_queue,
> > .dma_enable_flush_queue = arm_smmu_dma_enable_flush_queue,
> > - .domain_set_attr = arm_smmu_domain_set_attr,
> > + .domain_set_pgtable_attr = arm_smmu_domain_set_pgtable_attr,
> > .domain_enable_nesting = arm_smmu_domain_enable_nesting,
> > .of_xlate = arm_smmu_of_xlate,
> > .get_resv_regions = arm_smmu_get_resv_regions,
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 2e9e058501a953..8490aefd4b41f8 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2693,6 +2693,15 @@ int iommu_domain_enable_nesting(struct iommu_domain *domain)
> > }
> > EXPORT_SYMBOL_GPL(iommu_domain_enable_nesting);
> >
> > +int iommu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg)
> > +{
> > + if (!domain->ops->domain_set_pgtable_attr)
> > + return -EINVAL;
> > + return domain->ops->domain_set_pgtable_attr(domain, pgtbl_cfg);
> > +}
> > +EXPORT_SYMBOL_GPL(iommu_domain_set_pgtable_attr);
> > +
> > void iommu_get_resv_regions(struct device *dev, struct list_head *list)
> > {
> > const struct iommu_ops *ops = dev->bus->iommu_ops;
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index aed88aa3bd3edf..39d3ed4d2700ac 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -40,6 +40,7 @@ struct iommu_domain;
> > struct notifier_block;
> > struct iommu_sva;
> > struct iommu_fault_event;
> > +struct io_pgtable_domain_attr;
> >
> > /* iommu fault flags */
> > #define IOMMU_FAULT_READ 0x0
> > @@ -107,7 +108,6 @@ enum iommu_cap {
> > */
> >
> > enum iommu_attr {
> > - DOMAIN_ATTR_IO_PGTABLE_CFG,
> > DOMAIN_ATTR_MAX,
> > };
> >
> > @@ -196,6 +196,7 @@ struct iommu_iotlb_gather {
> > * @dma_enable_flush_queue: Try to enable the DMA flush queue
> > * @domain_set_attr: Change domain attributes
> > * @domain_enable_nesting: Enable nesting
> > + * @domain_set_pgtable_attr: Set io page table attributes
> > * @get_resv_regions: Request list of reserved regions for a device
> > * @put_resv_regions: Free list of reserved regions for a device
> > * @apply_resv_region: Temporary helper call-back for iova reserved ranges
> > @@ -249,6 +250,8 @@ struct iommu_ops {
> > int (*domain_set_attr)(struct iommu_domain *domain,
> > enum iommu_attr attr, void *data);
> > int (*domain_enable_nesting)(struct iommu_domain *domain);
> > + int (*domain_set_pgtable_attr)(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg);
> >
> > /* Request/Free a list of reserved regions for a device */
> > void (*get_resv_regions)(struct device *dev, struct list_head *list);
> > @@ -493,9 +496,13 @@ extern int iommu_group_id(struct iommu_group *group);
> > extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
> >
> > bool iommu_dma_use_flush_queue(struct iommu_domain *domain);
> > +int iommu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg);
> > extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
> > void *data);
> > int iommu_domain_enable_nesting(struct iommu_domain *domain);
> > +int iommu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg);
> >
> > extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
> > unsigned long iova, int flags);
> >
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply
* linux-next: manual merge of the tty tree with the powerpc-fixes tree
From: Stephen Rothwell @ 2021-03-05 1:05 UTC (permalink / raw)
To: Greg KH, Michael Ellerman, PowerPC
Cc: Uwe Kleine-König, Jiri Slaby, Greg Kroah-Hartman,
Linux Kernel Mailing List, Linux Next Mailing List, Jiri Slaby
[-- Attachment #1: Type: text/plain, Size: 802 bytes --]
Hi all,
Today's linux-next merge of the tty tree got a conflict in:
drivers/tty/hvc/hvcs.c
between commit:
386a966f5ce7 ("vio: make remove callback return void")
from the powerpc-fixes tree and commit:
fb8d350c291c ("tty: hvc, drop unneeded forward declarations")
from the tty tree.
I fixed it up (they both removed the forward decalrartion of
hvcs_remove(), but the latter removed more) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 1:43 UTC (permalink / raw)
To: mpe, benh, paulus, drt, ljp, sukadev, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
From: Junlin Yang <yangjunlin@yulong.com>
ibmvnic_remove locks multiple spinlocks while disabling interrupts:
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock_irqsave(&adapter->rwi_lock, flags);
there is no need for the second irqsave,since interrupts are disabled
at that point, so remove the second irqsave:
spin_lock_irqsave(&adapter->state_lock, flags);
spin_lock(&adapter->rwi_lock);
Generated by: ./scripts/coccinelle/locks/flags.cocci
./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
ERROR: nested lock+irqsave that reuses flags from line 5404.
Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 2464c8a..a52668d 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
* after setting state, so __ibmvnic_reset() which is called
* from the flush_work() below, can make progress.
*/
- spin_lock_irqsave(&adapter->rwi_lock, flags);
+ spin_lock(&adapter->rwi_lock);
adapter->state = VNIC_REMOVING;
- spin_unlock_irqrestore(&adapter->rwi_lock, flags);
+ spin_unlock(&adapter->rwi_lock);
spin_unlock_irqrestore(&adapter->state_lock, flags);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v2 7/7] ASoC: dt-bindings: imx-rpmsg: Add binding doc for rpmsg machine driver
From: Shengjiu Wang @ 2021-03-05 2:55 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, linux-kernel, Nicolin Chen,
Mark Brown, linuxppc-dev
In-Reply-To: <CAL_JsqK1uc82hfdE4yj0ye-D6vygiqWkDVW96NOb-8kEFVqHMg@mail.gmail.com>
Hi
On Fri, Mar 5, 2021 at 4:05 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Feb 18, 2021 at 1:23 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >
> > On Thu, Feb 11, 2021 at 6:18 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Sun, Feb 07, 2021 at 06:23:55PM +0800, Shengjiu Wang wrote:
> > > > Imx-rpmsg is a new added machine driver for supporting audio on Cortex-M
> > > > core. The Cortex-M core will control the audio interface, DMA and audio
> > > > codec, setup the pipeline, the audio driver on Cortex-A core side is just
> > > > to communitcate with M core, it is a virtual sound card and don't touch
> > > > the hardware.
> > >
> > > I don't understand why there are 2 nodes for this other than you happen
> > > to want to split this into 2 Linux drivers. It's 1 h/w thing.
> >
> > This one is for the sound card machine driver. Another one is
> > for the sound card cpu dai driver. so there are 2 nodes.
>
> You are explaining this to me in terms of drivers. Explain it in terms
> of h/w blocks.
>
Yes, there is only 1 h/w block, which is (MU) message unit
As the sound card needs a cpu dai node and sound card node,
so from the driver's perspective, I use two nodes.
Seems It is hard to only use one node for my case.
or do you have any suggestions?
Best regards
Wang shengjiu
^ permalink raw reply
* [PATCH V3] mm: Generalize HUGETLB_PAGE_SIZE_VARIABLE
From: Anshuman Khandual @ 2021-03-05 3:28 UTC (permalink / raw)
To: linux-mm
Cc: linux-ia64, Anshuman Khandual, linux-kernel, Paul Mackerras,
Andrew Morton, linuxppc-dev, Christoph Hellwig
HUGETLB_PAGE_SIZE_VARIABLE need not be defined for each individual
platform subscribing it. Instead just make it generic.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: linux-ia64@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This change was originally suggested in an earilier discussion. This
applies on v5.12-rc1 and has been build tested on all applicable
platforms i.e ia64 and powerpc.
https://patchwork.kernel.org/project/linux-mm/patch/1613024531-19040-3-git-send-email-anshuman.khandual@arm.com/
Changes in V3:
- Dropped the bool desciption that enabled user selection
- Dropped the dependency on HUGETLB_PAGE for HUGETLB_PAGE_SIZE_VARIABLE
Changes in V2:
https://patchwork.kernel.org/project/linux-mm/patch/1614661987-23881-1-git-send-email-anshuman.khandual@arm.com/
- Added a description for HUGETLB_PAGE_SIZE_VARIABLE
- Added HUGETLB_PAGE dependency while selecting HUGETLB_PAGE_SIZE_VARIABLE
Changes in V1:
https://patchwork.kernel.org/project/linux-mm/patch/1614577853-7452-1-git-send-email-anshuman.khandual@arm.com/
arch/ia64/Kconfig | 6 +-----
arch/powerpc/Kconfig | 6 +-----
mm/Kconfig | 7 +++++++
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 2ad7a8d29fcc..dccf5bfebf48 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -32,6 +32,7 @@ config IA64
select TTY
select HAVE_ARCH_TRACEHOOK
select HAVE_VIRT_CPU_ACCOUNTING
+ select HUGETLB_PAGE_SIZE_VARIABLE if HUGETLB_PAGE
select VIRT_TO_BUS
select GENERIC_IRQ_PROBE
select GENERIC_PENDING_IRQ if SMP
@@ -82,11 +83,6 @@ config STACKTRACE_SUPPORT
config GENERIC_LOCKBREAK
def_bool n
-config HUGETLB_PAGE_SIZE_VARIABLE
- bool
- depends on HUGETLB_PAGE
- default y
-
config GENERIC_CALIBRATE_DELAY
bool
default y
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 386ae12d8523..11fea95a1f2c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -231,6 +231,7 @@ config PPC
select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
select HAVE_PERF_REGS
select HAVE_PERF_USER_STACK_DUMP
+ select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE
select MMU_GATHER_RCU_TABLE_FREE
select MMU_GATHER_PAGE_SIZE
select HAVE_REGS_AND_STACK_ACCESS_API
@@ -415,11 +416,6 @@ config HIGHMEM
source "kernel/Kconfig.hz"
-config HUGETLB_PAGE_SIZE_VARIABLE
- bool
- depends on HUGETLB_PAGE && PPC_BOOK3S_64
- default y
-
config MATH_EMULATION
bool "Math emulation"
depends on 4xx || PPC_8xx || PPC_MPC832x || BOOKE
diff --git a/mm/Kconfig b/mm/Kconfig
index 24c045b24b95..4413a69e7850 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -274,6 +274,13 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
config ARCH_ENABLE_THP_MIGRATION
bool
+config HUGETLB_PAGE_SIZE_VARIABLE
+ def_bool n
+ help
+ Allows the pageblock_order value to be dynamic instead of just standard
+ HUGETLB_PAGE_ORDER when there are multiple HugeTLB page sizes available
+ on a platform.
+
config CONTIG_ALLOC
def_bool (MEMORY_ISOLATION && COMPACTION) || CMA
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Sukadev Bhattiprolu @ 2021-03-05 4:19 UTC (permalink / raw)
To: angkery
Cc: Junlin Yang, linux-kernel, kuba, netdev, ljp, drt, paulus,
linuxppc-dev, davem
In-Reply-To: <20210305014350.1460-1-angkery@163.com>
angkery [angkery@163.com] wrote:
> From: Junlin Yang <yangjunlin@yulong.com>
>
> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock_irqsave(&adapter->rwi_lock, flags);
>
> there is no need for the second irqsave,since interrupts are disabled
> at that point, so remove the second irqsave:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock(&adapter->rwi_lock);
>
> Generated by: ./scripts/coccinelle/locks/flags.cocci
> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> ERROR: nested lock+irqsave that reuses flags from line 5404.
>
Thanks. Please add
Fixes: 4a41c421f367 ("ibmvnic: serialize access to work queue on remove")
> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
Reviewed-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH v2 01/37] KVM: PPC: Book3S 64: remove unused kvmppc_h_protect argument
From: Daniel Axtens @ 2021-03-05 4:45 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1614383256.cikqwycx8o.astroid@bobo.none>
Hi Nick,
>> ERROR: code indent should use tabs where possible
>> #25: FILE: arch/powerpc/include/asm/kvm_ppc.h:770:
>> + unsigned long pte_index, unsigned long avpn);$
>>
>> WARNING: please, no spaces at the start of a line
>> #25: FILE: arch/powerpc/include/asm/kvm_ppc.h:770:
>> + unsigned long pte_index, unsigned long avpn);$
>
> All the declarations are using the same style in this file so I think
> I'll leave it for someone to do a cleanup patch on. Okay?
Huh, right you are. In that case:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
^ permalink raw reply
* Re: [PATCH v2 05/37] KVM: PPC: Book3S HV: Ensure MSR[ME] is always set in guest MSR
From: Daniel Axtens @ 2021-03-05 4:50 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Fabiano Rosas
In-Reply-To: <1614383549.rxe6rxw7w8.astroid@bobo.none>
>> This 'if' is technically redundant but you mention a future patch warning
>> on !(msr & MSR_ME) so I'm holding off on any judgement about the 'if' until
>> I get to that patch :)
>
> That's true. The warning is actually further down when we're setting up
> the msr to run in guest mode. At this point the MSR I think comes from
> qemu (and arguably the guest setup code shouldn't need to know about HV
> specific MSR bits) so a warning here wouldn't be appropriate.
>
> I could remove the if, although the compiler might already do that.
Yes, I think the compiler is almost certainly going to remove that.
I'd probably not include the 'if' statement even though it probably gets
removed by the compiler but I think that's more a matter of taste than
anything else.
Kind regards,
Daniel
>
>>
>> The patch seems sane to me, I agree that we don't want guests running with
>> MSR_ME=0 and kvmppc_set_msr_hv already ensures that the transactional state is
>> sane so this is another sanity-enforcement in the same sort of vein.
>>
>> All up:
>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Michael Ellerman @ 2021-03-05 5:01 UTC (permalink / raw)
To: Marco Elver, Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <YEDXJ5JNkgvDFehc@elver.google.com>
Marco Elver <elver@google.com> writes:
> On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
>> Le 04/03/2021 à 12:31, Marco Elver a écrit :
>> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
>> > <christophe.leroy@csgroup.eu> wrote:
>> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
>> > > >
>> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
>> > > > was printed along the KFENCE report above) didn't include the top
>> > > > frame in the "Call Trace", so this assumption is definitely not
>> > > > isolated to KFENCE.
>> > > >
>> > >
>> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
>> > > applied), and I get many failures. Any idea ?
>> > >
>> > > [ 17.653751][ T58] ==================================================================
>> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.654379][ T58]
>> > > [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
>> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
>> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
>> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
>> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
>> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
>> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
>> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
>> > > [ 17.659869][ T58]
> [...]
>> >
>> > Looks like something is prepending '.' to function names. We expect
>> > the function name to appear as-is, e.g. "kfence_guarded_free",
>> > "test_double_free", etc.
>> >
>> > Is there something special on ppc64, where the '.' is some convention?
>> >
>>
>> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
>>
>> Also see commit https://github.com/linuxppc/linux/commit/02424d896
>
> Thanks -- could you try the below patch? You'll need to define
> ARCH_FUNC_PREFIX accordingly.
>
> We think, since there are only very few architectures that add a prefix,
> requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> the simplest option. Let me know if this works for you.
>
> There an alternative option, which is to dynamically figure out the
> prefix, but if this simpler option is fine with you, we'd prefer it.
We have rediscovered this problem in basically every tracing / debugging
feature added in the last 20 years :)
I think the simplest solution is the one tools/perf/util/symbol.c uses,
which is to just skip a leading '.'.
Does that work?
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index ab83d5a59bb1..67b49dc54b38 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
for (skipnr = 0; skipnr < num_entries; skipnr++) {
int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ if (buf[0] == '.')
+ buf++;
+
if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
!strncmp(buf, "__slab_free", len)) {
/*
cheers
^ permalink raw reply related
* Re: [PATCH v2 06/37] KVM: PPC: Book3S 64: move KVM interrupt entry to a common entry point
From: Daniel Axtens @ 2021-03-05 5:03 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-7-npiggin@gmail.com>
Hi Nick,
> Rather than bifurcate the call depending on whether or not HV is
> possible, and have the HV entry test for PR, just make a single
> common point which does the demultiplexing. This makes it simpler
> to add another type of exit handler.
>
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
I checked this against the RFC which I was happy with and there are no
changes that I am concerned about. The expanded comment is also nice.
As such:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 8 +-----
> arch/powerpc/kvm/Makefile | 3 +++
> arch/powerpc/kvm/book3s_64_entry.S | 35 +++++++++++++++++++++++++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 11 ++------
> 4 files changed, 41 insertions(+), 16 deletions(-)
> create mode 100644 arch/powerpc/kvm/book3s_64_entry.S
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 0097e0676ed7..ba13d749d203 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -208,7 +208,6 @@ do_define_int n
> .endm
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> /*
> * All interrupts which set HSRR registers, as well as SRESET and MCE and
> * syscall when invoked with "sc 1" switch to MSR[HV]=1 (HVMODE) to be taken,
> @@ -238,13 +237,8 @@ do_define_int n
>
> /*
> * If an interrupt is taken while a guest is running, it is immediately routed
> - * to KVM to handle. If both HV and PR KVM arepossible, KVM interrupts go first
> - * to kvmppc_interrupt_hv, which handles the PR guest case.
> + * to KVM to handle.
> */
> -#define kvmppc_interrupt kvmppc_interrupt_hv
> -#else
> -#define kvmppc_interrupt kvmppc_interrupt_pr
> -#endif
>
> .macro KVMTEST name
> lbz r10,HSTATE_IN_GUEST(r13)
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index 2bfeaa13befb..cdd119028f64 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -59,6 +59,9 @@ kvm-pr-y := \
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> tm.o
>
> +kvm-book3s_64-builtin-objs-y += \
> + book3s_64_entry.o
> +
> ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
> book3s_rmhandlers.o
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> new file mode 100644
> index 000000000000..e9a6a8fbb164
> --- /dev/null
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -0,0 +1,35 @@
> +#include <asm/asm-offsets.h>
> +#include <asm/cache.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_book3s_asm.h>
> +#include <asm/ppc_asm.h>
> +#include <asm/reg.h>
> +
> +/*
> + * This is branched to from interrupt handlers in exception-64s.S which set
> + * IKVM_REAL or IKVM_VIRT, if HSTATE_IN_GUEST was found to be non-zero.
> + */
> +.global kvmppc_interrupt
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_interrupt:
> + /*
> + * Register contents:
> + * R12 = (guest CR << 32) | interrupt vector
> + * R13 = PACA
> + * guest R12 saved in shadow VCPU SCRATCH0
> + * guest R13 saved in SPRN_SCRATCH0
> + */
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + std r9,HSTATE_SCRATCH2(r13)
> + lbz r9,HSTATE_IN_GUEST(r13)
> + cmpwi r9,KVM_GUEST_MODE_HOST_HV
> + beq kvmppc_bad_host_intr
> +#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> + cmpwi r9,KVM_GUEST_MODE_GUEST
> + ld r9,HSTATE_SCRATCH2(r13)
> + beq kvmppc_interrupt_pr
> +#endif
> + b kvmppc_interrupt_hv
> +#else
> + b kvmppc_interrupt_pr
> +#endif
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 5e634db4809b..f976efb7e4a9 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1269,16 +1269,8 @@ kvmppc_interrupt_hv:
> * R13 = PACA
> * guest R12 saved in shadow VCPU SCRATCH0
> * guest R13 saved in SPRN_SCRATCH0
> + * guest R9 saved in HSTATE_SCRATCH2
> */
> - std r9, HSTATE_SCRATCH2(r13)
> - lbz r9, HSTATE_IN_GUEST(r13)
> - cmpwi r9, KVM_GUEST_MODE_HOST_HV
> - beq kvmppc_bad_host_intr
> -#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> - cmpwi r9, KVM_GUEST_MODE_GUEST
> - ld r9, HSTATE_SCRATCH2(r13)
> - beq kvmppc_interrupt_pr
> -#endif
> /* We're now back in the host but in guest MMU context */
> li r9, KVM_GUEST_MODE_HOST_HV
> stb r9, HSTATE_IN_GUEST(r13)
> @@ -3280,6 +3272,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_P9_TM_HV_ASSIST)
> * cfar is saved in HSTATE_CFAR(r13)
> * ppr is saved in HSTATE_PPR(r13)
> */
> +.global kvmppc_bad_host_intr
> kvmppc_bad_host_intr:
> /*
> * Switch to the emergency stack, but start half-way down in
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Christophe Leroy @ 2021-03-05 5:49 UTC (permalink / raw)
To: angkery, mpe, benh, paulus, drt, ljp, sukadev, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
In-Reply-To: <20210305014350.1460-1-angkery@163.com>
Le 05/03/2021 à 02:43, angkery a écrit :
> From: Junlin Yang <yangjunlin@yulong.com>
>
> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock_irqsave(&adapter->rwi_lock, flags);
>
> there is no need for the second irqsave,since interrupts are disabled
> at that point, so remove the second irqsave:
The problème is not that there is no need. The problem is a lot more serious:
As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first
_irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value
saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is
_THE_ problem really.
> spin_lock_irqsave(&adapter->state_lock, flags);
> spin_lock(&adapter->rwi_lock);
>
> Generated by: ./scripts/coccinelle/locks/flags.cocci
> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> ERROR: nested lock+irqsave that reuses flags from line 5404.
>
> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> ---
> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index 2464c8a..a52668d 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
> * after setting state, so __ibmvnic_reset() which is called
> * from the flush_work() below, can make progress.
> */
> - spin_lock_irqsave(&adapter->rwi_lock, flags);
> + spin_lock(&adapter->rwi_lock);
> adapter->state = VNIC_REMOVING;
> - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> + spin_unlock(&adapter->rwi_lock);
>
> spin_unlock_irqrestore(&adapter->state_lock, flags);
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/perf: prevent mixed EBB and non-EBB events
From: Athira Rajeev @ 2021-03-05 5:50 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: Peter Zijlstra, linuxppc-dev, linux-kernel
In-Reply-To: <20210224122116.221120-1-cascardo@canonical.com>
> On 24-Feb-2021, at 5:51 PM, Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:
>
> EBB events must be under exclusive groups, so there is no mix of EBB and
> non-EBB events on the same PMU. This requirement worked fine as perf core
> would not allow other pinned events to be scheduled together with exclusive
> events.
>
> This assumption was broken by commit 1908dc911792 ("perf: Tweak
> perf_event_attr::exclusive semantics").
>
> After that, the test cpu_event_pinned_vs_ebb_test started succeeding after
> read_events, but worse, the task would not have given access to PMC1, so
> when it tried to write to it, it was killed with "illegal instruction".
>
> Preventing mixed EBB and non-EBB events from being add to the same PMU will
> just revert to the previous behavior and the test will succeed.
Hi,
Thanks for checking this. I checked your patch which is fixing “check_excludes” to make
sure all events must agree on EBB. But in the PMU group constraints, we already have check for
EBB events. This is in arch/powerpc/perf/isa207-common.c ( isa207_get_constraint function ).
<<>>
mask |= CNST_EBB_VAL(ebb);
value |= CNST_EBB_MASK;
<<>>
But the above setting for mask and value is interchanged. We actually need to fix here.
Below patch should fix this:
diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
index e4f577da33d8..8b5eeb6fb2fb 100644
--- a/arch/powerpc/perf/isa207-common.c
+++ b/arch/powerpc/perf/isa207-common.c
@@ -447,8 +447,8 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp,
* EBB events are pinned & exclusive, so this should never actually
* hit, but we leave it as a fallback in case.
*/
- mask |= CNST_EBB_VAL(ebb);
- value |= CNST_EBB_MASK;
+ mask |= CNST_EBB_MASK;
+ value |= CNST_EBB_VAL(ebb);
*maskp = mask;
*valp = value;
Can you please try with this patch.
Thanks
Athira
>
> Fixes: 1908dc911792 (perf: Tweak perf_event_attr::exclusive semantics)
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
> arch/powerpc/perf/core-book3s.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 43599e671d38..d767f7944f85 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1010,9 +1010,25 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> int n_prev, int n_new)
> {
> int eu = 0, ek = 0, eh = 0;
> + bool ebb = false;
> int i, n, first;
> struct perf_event *event;
>
> + n = n_prev + n_new;
> + if (n <= 1)
> + return 0;
> +
> + first = 1;
> + for (i = 0; i < n; ++i) {
> + event = ctrs[i];
> + if (first) {
> + ebb = is_ebb_event(event);
> + first = 0;
> + } else if (is_ebb_event(event) != ebb) {
> + return -EAGAIN;
> + }
> + }
> +
> /*
> * If the PMU we're on supports per event exclude settings then we
> * don't need to do any of this logic. NB. This assumes no PMU has both
> @@ -1021,10 +1037,6 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
> if (ppmu->flags & PPMU_ARCH_207S)
> return 0;
>
> - n = n_prev + n_new;
> - if (n <= 1)
> - return 0;
> -
> first = 1;
> for (i = 0; i < n; ++i) {
> if (cflags[i] & PPMU_LIMITED_PMC_OK) {
> --
> 2.27.0
>
^ permalink raw reply related
* Re: [PATCH v2 07/37] KVM: PPC: Book3S 64: Move GUEST_MODE_SKIP test into KVM
From: Daniel Axtens @ 2021-03-05 5:54 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-8-npiggin@gmail.com>
Hi Nick,
> - .if IKVM_SKIP
> -89: mtocrf 0x80,r9
> - ld r10,IAREA+EX_CTR(r13)
> - mtctr r10
> - ld r9,IAREA+EX_R9(r13)
> - ld r10,IAREA+EX_R10(r13)
> - ld r11,IAREA+EX_R11(r13)
> - ld r12,IAREA+EX_R12(r13)
> - .if IHSRR_IF_HVMODE
> - BEGIN_FTR_SECTION
> - b kvmppc_skip_Hinterrupt
> - FTR_SECTION_ELSE
> - b kvmppc_skip_interrupt
> - ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
I was initially concerned that you were pulling out the complexities
around IHSRR_IF_HVMODE, but I checked exceptions-64s.S and the only
exception handler that sets IHSRR_IF_HVMODE is hardware_interrupt and
that does not set IKVM_SKIP, so we are indeed fine to not keep this
complex little asm section.
> - .elseif IHSRR
> - b kvmppc_skip_Hinterrupt
> - .else
> - b kvmppc_skip_interrupt
> - .endif
> - .endif
> .endm
> +/*
> + * KVM uses a trick where it is running in MSR[HV]=1 mode in real-mode with the
> + * guest MMU context loaded, and it sets KVM_GUEST_MODE_SKIP and enables
> + * MSR[DR]=1 while leaving MSR[IR]=0, so it continues to fetch HV instructions
> + * but loads and stores will access the guest context. This is used to load
> + * the faulting instruction without walking page tables.
> + *
> + * However the guest context may not be able to translate, or it may cause a
> + * machine check or other issue, which will result in a fault in the host
> + * (even with KVM-HV).
> + *
> + * These faults are caught here and if the fault was (or was likely) due to
> + * that load, then we just return with the PC advanced +4 and skip the load,
> + * which then goes via the slow path.
> + */
This is a really helpful comment! Thanks!
> +.Lmaybe_skip:
> + cmpwi r12,BOOK3S_INTERRUPT_MACHINE_CHECK
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_STORAGE
> + beq 1f
> + cmpwi r12,BOOK3S_INTERRUPT_DATA_SEGMENT
> + beq 1f
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> + /* HSRR interrupts have 2 added to trap vector */
> + cmpwi r12,BOOK3S_INTERRUPT_H_DATA_STORAGE | 0x2
This took me by surprise for a while, but I checked how it works in
exceptions-64s.S and indeed the GEN_KVM macro will add 0x2 to the IVEC
if IHSRR is set, and it is set for h_data_storage.
So this checks out to me.
I have checked, to the best of my limited assembler capabilities that
the logic before and after matches. It seems good to me.
On that limited basis,
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> + beq 2f
> +#endif
> + b .Lno_skip
> +1: mfspr r9,SPRN_SRR0
> + addi r9,r9,4
> + mtspr SPRN_SRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + RFI_TO_KERNEL
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +2: mfspr r9,SPRN_HSRR0
> + addi r9,r9,4
> + mtspr SPRN_HSRR0,r9
> + ld r12,HSTATE_SCRATCH0(r13)
> + ld r9,HSTATE_SCRATCH2(r13)
> + GET_SCRATCH0(r13)
> + HRFI_TO_KERNEL
> +#endif
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v2 08/37] KVM: PPC: Book3S 64: add hcall interrupt handler
From: Daniel Axtens @ 2021-03-05 6:03 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210225134652.2127648-9-npiggin@gmail.com>
Hi Nick,
> Add a separate hcall entry point. This can be used to deal with the
> different calling convention.
Looks good to me, makes sense, passes checkpatch.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 4 ++--
> arch/powerpc/kvm/book3s_64_entry.S | 6 +++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index d956dd9ed61f..9ae463e8522b 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1992,13 +1992,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> * outside the head section.
> */
> - __LOAD_FAR_HANDLER(r10, kvmppc_interrupt)
> + __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> ld r10,PACA_EXGEN+EX_R10(r13)
> - b kvmppc_interrupt
> + b kvmppc_hcall
> #endif
> #endif
>
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index c1276f616af4..9572f759255c 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -7,9 +7,13 @@
> #include <asm/reg.h>
>
> /*
> - * This is branched to from interrupt handlers in exception-64s.S which set
> + * These are branched to from interrupt handlers in exception-64s.S which set
> * IKVM_REAL or IKVM_VIRT, if HSTATE_IN_GUEST was found to be non-zero.
> */
> +.global kvmppc_hcall
> +.balign IFETCH_ALIGN_BYTES
> +kvmppc_hcall:
> +
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> kvmppc_interrupt:
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: Lijun Pan @ 2021-03-05 6:04 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, angkery, linux-kernel, Jakub Kicinski, netdev,
Lijun Pan, Dany Madden, paulus, Sukadev Bhattiprolu, linuxppc-dev,
David Miller
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
> On Mar 4, 2021, at 11:49 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 05/03/2021 à 02:43, angkery a écrit :
>> From: Junlin Yang <yangjunlin@yulong.com>
>> ibmvnic_remove locks multiple spinlocks while disabling interrupts:
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock_irqsave(&adapter->rwi_lock, flags);
>> there is no need for the second irqsave,since interrupts are disabled
>> at that point, so remove the second irqsave:
>
> The problème is not that there is no need. The problem is a lot more serious:
> As reported by coccinella, the second _irqsave() overwrites the value saved in 'flags' by the first _irqsave, therefore when the second _irqrestore comes, the value in 'flags' is not valid, the value saved by the first _irqsave has been lost. This likely leads to IRQs remaining disabled, which is _THE_ problem really.
That does sounds like a more serious functional problem than coccinella check.
Thanks for your explanation.
>
>> spin_lock_irqsave(&adapter->state_lock, flags);
>> spin_lock(&adapter->rwi_lock);
>> Generated by: ./scripts/coccinelle/locks/flags.cocci
>> ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
>> ERROR: nested lock+irqsave that reuses flags from line 5404.
>> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
>> ---
>> drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 2464c8a..a52668d 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev *dev)
>> * after setting state, so __ibmvnic_reset() which is called
>> * from the flush_work() below, can make progress.
>> */
>> - spin_lock_irqsave(&adapter->rwi_lock, flags);
>> + spin_lock(&adapter->rwi_lock);
>> adapter->state = VNIC_REMOVING;
>> - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
>> + spin_unlock(&adapter->rwi_lock);
>> spin_unlock_irqrestore(&adapter->state_lock, flags);
>>
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: export LPAR security flavor in lparcfg
From: Michael Ellerman @ 2021-03-05 6:23 UTC (permalink / raw)
To: Laurent Dufour, benh, paulus, linuxppc-dev; +Cc: nathanl, cheloha, linux-kernel
In-Reply-To: <20210304114240.54112-1-ldufour@linux.ibm.com>
Laurent Dufour <ldufour@linux.ibm.com> writes:
> This is helpful to read the security flavor from inside the LPAR.
We already have /sys/kernel/debug/powerpc/security_features.
Is that not sufficient?
> Export it like this in /proc/powerpc/lparcfg:
>
> $ grep security_flavor /proc/powerpc/lparcfg
> security_flavor=1
>
> Value means:
> 0 Speculative execution fully enabled
> 1 Speculative execution controls to mitigate user-to-kernel attacks
> 2 Speculative execution controls to mitigate user-to-kernel and
> user-to-user side-channel attacks
Those strings come from the FSP help, but we have no guarantee it won't
mean something different in future.
cheers
^ permalink raw reply
* Re: linux-next: manual merge of the tty tree with the powerpc-fixes tree
From: Greg KH @ 2021-03-05 6:30 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Uwe Kleine-König, Jiri Slaby, PowerPC,
Linux Kernel Mailing List, Linux Next Mailing List, Jiri Slaby
In-Reply-To: <20210305120523.0cb114b9@canb.auug.org.au>
On Fri, Mar 05, 2021 at 12:05:23PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the tty tree got a conflict in:
>
> drivers/tty/hvc/hvcs.c
>
> between commit:
>
> 386a966f5ce7 ("vio: make remove callback return void")
>
> from the powerpc-fixes tree and commit:
>
> fb8d350c291c ("tty: hvc, drop unneeded forward declarations")
>
> from the tty tree.
>
> I fixed it up (they both removed the forward decalrartion of
> hvcs_remove(), but the latter removed more) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
Thanks, that's the correct fix, we knew this was going to happen :(
greg k-h
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Christophe Leroy @ 2021-03-05 6:38 UTC (permalink / raw)
To: Segher Boessenkool, Nick Desaulniers
Cc: Mark Rutland, Marco Elver, Will Deacon, Catalin Marinas, LKML,
kasan-dev, Mark Brown, Paul Mackerras, linux-toolchains,
linuxppc-dev, Linux ARM
In-Reply-To: <20210304192447.GT29191@gate.crashing.org>
Le 04/03/2021 à 20:24, Segher Boessenkool a écrit :
> On Thu, Mar 04, 2021 at 09:54:44AM -0800, Nick Desaulniers wrote:
>> On Thu, Mar 4, 2021 at 9:42 AM Marco Elver <elver@google.com> wrote:
>> include/linux/compiler.h:246:
>> prevent_tail_call_optimization
>>
>> commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
https://github.com/linuxppc/linux/commit/a9a3ed1eff36
>
> That is much heavier than needed (an mb()). You can just put an empty
> inline asm after a call before a return, and that call cannot be
> optimised to a sibling call: (the end of a function is an implicit
> return:)
>
> Instead of:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> }
>
> Do:
>
> void g(void);
> void f(int x)
> if (x)
> g();
> asm("");
> }
>
> This costs no extra instructions, and certainly not something as heavy
> as an mb()! It works without the "if" as well, of course, but with it
> it is a more interesting example of a tail call.
In the commit mentionned at the top, it is said:
The next attempt to prevent compilers from tail-call optimizing
the last function call cpu_startup_entry(), ... , was to add an empty asm("").
This current solution was short and sweet, and reportedly, is supported
by both compilers but we didn't get very far this time: future (LTO?)
optimization passes could potentially eliminate this, which leads us
to the third attempt: having an actual memory barrier there which the
compiler cannot ignore or move around etc.
Christophe
^ permalink raw reply
* Re: [PATCH v2 09/37] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
From: Daniel Axtens @ 2021-03-05 6:48 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210225134652.2127648-10-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> System calls / hcalls have a different calling convention than
> other interrupts, so there is code in the KVMTEST to massage these
> into the same form as other interrupt handlers.
>
> Move this work into the KVM hcall handler. This means teaching KVM
> a little more about the low level interrupt handler setup, PACA save
> areas, etc., although that's not obviously worse than the current
> approach of coming up with an entirely different interrupt register
> / save convention.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/exception-64s.h | 13 +++++++
> arch/powerpc/kernel/exceptions-64s.S | 44 ++----------------------
> arch/powerpc/kvm/book3s_64_entry.S | 17 +++++++++
> 3 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
> index c1a8aac01cf9..bb6f78fcf981 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -35,6 +35,19 @@
> /* PACA save area size in u64 units (exgen, exmc, etc) */
> #define EX_SIZE 10
>
> +/* PACA save area offsets */
> +#define EX_R9 0
> +#define EX_R10 8
> +#define EX_R11 16
> +#define EX_R12 24
> +#define EX_R13 32
> +#define EX_DAR 40
> +#define EX_DSISR 48
> +#define EX_CCR 52
> +#define EX_CFAR 56
> +#define EX_PPR 64
> +#define EX_CTR 72
I wonder - now that EX_blah and EX_SIZE both live in the one header
file, if we could calculate EX_SIZE based on the last offset... or have
a BUILD_BUG_ON or something? ... just trying not to lose the sanity
checking afforded to us by the .if macro we lose when moving these out
of exceptions-64s.S...
> +
> /*
> * maximum recursive depth of MCE exceptions
> */
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 9ae463e8522b..b7092ba87da8 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -21,22 +21,6 @@
> #include <asm/feature-fixups.h>
> #include <asm/kup.h>
>
> -/* PACA save area offsets (exgen, exmc, etc) */
> -#define EX_R9 0
> -#define EX_R10 8
> -#define EX_R11 16
> -#define EX_R12 24
> -#define EX_R13 32
> -#define EX_DAR 40
> -#define EX_DSISR 48
> -#define EX_CCR 52
> -#define EX_CFAR 56
> -#define EX_PPR 64
> -#define EX_CTR 72
> -.if EX_SIZE != 10
> - .error "EX_SIZE is wrong"
> -.endif
> -
> /*
> * Following are fixed section helper macros.
> *
> @@ -1964,45 +1948,21 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>
> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> TRAMP_REAL_BEGIN(system_call_kvm)
> - /*
> - * This is a hcall, so register convention is as above, with these
> - * differences:
> - * r13 = PACA
> - * ctr = orig r13
> - * orig r10 saved in PACA
> - */
> - /*
> - * Save the PPR (on systems that support it) before changing to
> - * HMT_MEDIUM. That allows the KVM code to save that value into the
> - * guest state (it is the guest's PPR value).
> - */
> -BEGIN_FTR_SECTION
> - mfspr r10,SPRN_PPR
> - std r10,HSTATE_PPR(r13)
> -END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> - HMT_MEDIUM
> mfctr r10
> - SET_SCRATCH0(r10)
> - mfcr r10
> - std r12,HSTATE_SCRATCH0(r13)
> - sldi r12,r10,32
> - ori r12,r12,0xc00
> + SET_SCRATCH0(r10) /* Save r13 in SCRATCH0 */
> #ifdef CONFIG_RELOCATABLE
> /*
> - * Requires __LOAD_FAR_HANDLER beause kvmppc_interrupt lives
> + * Requires __LOAD_FAR_HANDLER beause kvmppc_hcall lives
That change probably belongs in the previous patch :)
Beyond that, I'm still wrapping my head around the deep magic going on
here! Hopefully I'll get a bit a bit further next time I look.
Kind regards,
Daniel
> * outside the head section.
> */
> __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
> mtctr r10
> - ld r10,PACA_EXGEN+EX_R10(r13)
> bctr
> #else
> - ld r10,PACA_EXGEN+EX_R10(r13)
> b kvmppc_hcall
> #endif
> #endif
>
> -
> /**
> * Interrupt 0xd00 - Trace Interrupt.
> * This is a synchronous interrupt in response to instruction step or
> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
> index 9572f759255c..1c9518ab7d96 100644
> --- a/arch/powerpc/kvm/book3s_64_entry.S
> +++ b/arch/powerpc/kvm/book3s_64_entry.S
> @@ -13,6 +13,23 @@
> .global kvmppc_hcall
> .balign IFETCH_ALIGN_BYTES
> kvmppc_hcall:
> + /*
> + * This is a hcall, so register convention is as
> + * Documentation/powerpc/papr_hcalls.rst, with these additions:
> + * R13 = PACA
> + * guest R13 saved in SPRN_SCRATCH0
> + * R10 = free
> + */
> +BEGIN_FTR_SECTION
> + mfspr r10,SPRN_PPR
> + std r10,HSTATE_PPR(r13)
> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> + HMT_MEDIUM
> + mfcr r10
> + std r12,HSTATE_SCRATCH0(r13)
> + sldi r12,r10,32
> + ori r12,r12,0xc00
> + ld r10,PACA_EXGEN+EX_R10(r13)
>
> .global kvmppc_interrupt
> .balign IFETCH_ALIGN_BYTES
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH] ibmvnic: remove excessive irqsave
From: angkery @ 2021-03-05 7:03 UTC (permalink / raw)
To: Christophe Leroy
Cc: Junlin Yang, linux-kernel, kuba, netdev, ljp, drt, paulus,
sukadev, linuxppc-dev, davem
In-Reply-To: <67215668-0850-a0f3-06e1-49db590b8fcc@csgroup.eu>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=GB18030, Size: 2197 bytes --]
On Fri, 5 Mar 2021 06:49:14 +0100
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> Le 05/03/2021 ¨¤ 02:43, angkery a ¨¦crit0„2:
> > From: Junlin Yang <yangjunlin@yulong.com>
> >
> > ibmvnic_remove locks multiple spinlocks while disabling interrupts:
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock_irqsave(&adapter->rwi_lock, flags);
> >
> > there is no need for the second irqsave,since interrupts are
> > disabled at that point, so remove the second irqsave:
>
> The probl¨¨me is not that there is no need. The problem is a lot more
> serious: As reported by coccinella, the second _irqsave() overwrites
> the value saved in 'flags' by the first _irqsave, therefore when the
> second _irqrestore comes, the value in 'flags' is not valid, the
> value saved by the first _irqsave has been lost. This likely leads to
> IRQs remaining disabled, which is _THE_ problem really.
>
Thank you for explaining the real problem.
I will update the commit information with your description.
> > spin_lock_irqsave(&adapter->state_lock, flags);
> > spin_lock(&adapter->rwi_lock);
> >
> > Generated by: ./scripts/coccinelle/locks/flags.cocci
> > ./drivers/net/ethernet/ibm/ibmvnic.c:5413:1-18:
> > ERROR: nested lock+irqsave that reuses flags from line 5404.
> >
> > Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> > ---
> > drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c index 2464c8a..a52668d 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -5408,9 +5408,9 @@ static void ibmvnic_remove(struct vio_dev
> > *dev)
> > * after setting state, so __ibmvnic_reset() which is
> > called
> > * from the flush_work() below, can make progress.
> > */
> > - spin_lock_irqsave(&adapter->rwi_lock, flags);
> > + spin_lock(&adapter->rwi_lock);
> > adapter->state = VNIC_REMOVING;
> > - spin_unlock_irqrestore(&adapter->rwi_lock, flags);
> > + spin_unlock(&adapter->rwi_lock);
> >
> > spin_unlock_irqrestore(&adapter->state_lock, flags);
> >
> >
^ 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