* Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
From: Marco Elver @ 2021-03-04 14:19 UTC (permalink / raw)
To: Christophe Leroy
Cc: LKML, kasan-dev, Alexander Potapenko, Paul Mackerras,
linuxppc-dev, Dmitry Vyukov
In-Reply-To: <4b46ecc9-ae47-eee1-843e-e0638a356b51@csgroup.eu>
On Thu, 4 Mar 2021 at 15:08, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 04/03/2021 à 13:48, Marco Elver a écrit :
> > From d118080eb9552073f5dcf1f86198f3d86d5ea850 Mon Sep 17 00:00:00 2001
> > From: Marco Elver <elver@google.com>
> > Date: Thu, 4 Mar 2021 13:15:51 +0100
> > Subject: [PATCH] kfence: fix reports if constant function prefixes exist
> >
> > Some architectures prefix all functions with a constant string ('.' on
> > ppc64). Add ARCH_FUNC_PREFIX, which may optionally be defined in
> > <asm/kfence.h>, so that get_stack_skipnr() can work properly.
>
>
> It works, thanks.
>
> >
> > Link: https://lkml.kernel.org/r/f036c53d-7e81-763c-47f4-6024c6c5f058@csgroup.eu
> > Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Signed-off-by: Marco Elver <elver@google.com>
>
> Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Thanks, I'll send this to Andrew for inclusion in -mm, since this is
not a strict dependency (it'll work without the patch, just the stack
traces aren't that pretty but still useful). If the ppc patches and
this make it into the next merge window, everything should be good for
5.13.
> > ---
> > mm/kfence/report.c | 18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> > index 519f037720f5..e3f71451ad9e 100644
> > --- a/mm/kfence/report.c
> > +++ b/mm/kfence/report.c
> > @@ -20,6 +20,11 @@
> >
> > #include "kfence.h"
> >
> > +/* May be overridden by <asm/kfence.h>. */
> > +#ifndef ARCH_FUNC_PREFIX
> > +#define ARCH_FUNC_PREFIX ""
> > +#endif
> > +
> > extern bool no_hash_pointers;
> >
> > /* Helper function to either print to a seq_file or to console. */
> > @@ -67,8 +72,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 (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
> > - !strncmp(buf, "__slab_free", len)) {
> > + if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfence_") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "__kfence_") ||
> > + !strncmp(buf, ARCH_FUNC_PREFIX "__slab_free", len)) {
> > /*
> > * In case of tail calls from any of the below
> > * to any of the above.
> > @@ -77,10 +83,10 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> > }
> >
> > /* Also the *_bulk() variants by only checking prefixes. */
> > - if (str_has_prefix(buf, "kfree") ||
> > - str_has_prefix(buf, "kmem_cache_free") ||
> > - str_has_prefix(buf, "__kmalloc") ||
> > - str_has_prefix(buf, "kmem_cache_alloc"))
> > + if (str_has_prefix(buf, ARCH_FUNC_PREFIX "kfree") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_free") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "__kmalloc") ||
> > + str_has_prefix(buf, ARCH_FUNC_PREFIX "kmem_cache_alloc"))
> > goto found;
> > }
> > if (fallback < num_entries)
> >
^ permalink raw reply
* Re: [PATCH 2/5] CMDLINE: drivers: of: ifdef out cmdline section
From: Rob Herring @ 2021-03-04 14:32 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: <20210304044803.812204-2-danielwa@cisco.com>
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.
> /* Retrieve command line */
> p = of_get_flat_dt_prop(node, "bootargs", &l);
This needs to be outside the ifdef.
> +
> + /*
> + * 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 */
> + COMMAND_LINE_SIZE);
> +#else
> if (p != NULL && l > 0)
> strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
>
> @@ -1070,6 +1081,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> #endif
> #endif /* CONFIG_CMDLINE */
> +#endif /* CONFIG_GENERIC_CMDLINE */
>
> pr_debug("Command line is: %s\n", (char *)data);
>
> --
> 2.25.1
>
^ permalink raw reply
* [PATCH v2 3/4] powerpc/64s: Allow double call of kernel_[un]map_linear_page()
From: Christophe Leroy @ 2021-03-04 14:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Alexander Potapenko, Marco Elver, Dmitry Vyukov
Cc: linuxppc-dev, linux-kernel, kasan-dev
In-Reply-To: <8dfe1bd2abde26337c1d8c1ad0acfcc82185e0d5.1614868445.git.christophe.leroy@csgroup.eu>
If the page is already mapped resp. already unmapped, bail out.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
arch/powerpc/mm/book3s64/hash_utils.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index f1b5a5f1d3a9..cb09a49be798 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -1944,6 +1944,9 @@ static void kernel_map_linear_page(unsigned long vaddr, unsigned long lmi)
if (!vsid)
return;
+ if (linear_map_hash_slots[lmi] & 0x80)
+ return;
+
ret = hpte_insert_repeating(hash, vpn, __pa(vaddr), mode,
HPTE_V_BOLTED,
mmu_linear_psize, mmu_kernel_ssize);
@@ -1963,7 +1966,10 @@ static void kernel_unmap_linear_page(unsigned long vaddr, unsigned long lmi)
hash = hpt_hash(vpn, PAGE_SHIFT, mmu_kernel_ssize);
spin_lock(&linear_map_hash_lock);
- BUG_ON(!(linear_map_hash_slots[lmi] & 0x80));
+ if (!(linear_map_hash_slots[lmi] & 0x80)) {
+ spin_unlock(&linear_map_hash_lock);
+ return;
+ }
hidx = linear_map_hash_slots[lmi] & 0x7f;
linear_map_hash_slots[lmi] = 0;
spin_unlock(&linear_map_hash_lock);
--
2.25.0
^ permalink raw reply related
* [PATCH v2 2/4] powerpc/64s: Remove unneeded #ifdef CONFIG_DEBUG_PAGEALLOC in hash_utils
From: Christophe Leroy @ 2021-03-04 14:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Alexander Potapenko, Marco Elver, Dmitry Vyukov
Cc: linuxppc-dev, linux-kernel, kasan-dev
In-Reply-To: <8dfe1bd2abde26337c1d8c1ad0acfcc82185e0d5.1614868445.git.christophe.leroy@csgroup.eu>
debug_pagealloc_enabled() is always defined and constant folds to
'false' when CONFIG_DEBUG_PAGEALLOC is not enabled.
Remove the #ifdefs, the code and associated static variables will
be optimised out by the compiler when CONFIG_DEBUG_PAGEALLOC is
not defined.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
arch/powerpc/mm/book3s64/hash_utils.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 581b20a2feaf..f1b5a5f1d3a9 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -126,11 +126,8 @@ EXPORT_SYMBOL_GPL(mmu_slb_size);
#ifdef CONFIG_PPC_64K_PAGES
int mmu_ci_restrictions;
#endif
-#ifdef CONFIG_DEBUG_PAGEALLOC
static u8 *linear_map_hash_slots;
static unsigned long linear_map_hash_count;
-static DEFINE_SPINLOCK(linear_map_hash_lock);
-#endif /* CONFIG_DEBUG_PAGEALLOC */
struct mmu_hash_ops mmu_hash_ops;
EXPORT_SYMBOL(mmu_hash_ops);
@@ -326,11 +323,9 @@ int htab_bolt_mapping(unsigned long vstart, unsigned long vend,
break;
cond_resched();
-#ifdef CONFIG_DEBUG_PAGEALLOC
if (debug_pagealloc_enabled() &&
(paddr >> PAGE_SHIFT) < linear_map_hash_count)
linear_map_hash_slots[paddr >> PAGE_SHIFT] = ret | 0x80;
-#endif /* CONFIG_DEBUG_PAGEALLOC */
}
return ret < 0 ? ret : 0;
}
@@ -954,7 +949,6 @@ static void __init htab_initialize(void)
prot = pgprot_val(PAGE_KERNEL);
-#ifdef CONFIG_DEBUG_PAGEALLOC
if (debug_pagealloc_enabled()) {
linear_map_hash_count = memblock_end_of_DRAM() >> PAGE_SHIFT;
linear_map_hash_slots = memblock_alloc_try_nid(
@@ -964,7 +958,6 @@ static void __init htab_initialize(void)
panic("%s: Failed to allocate %lu bytes max_addr=%pa\n",
__func__, linear_map_hash_count, &ppc64_rma_size);
}
-#endif /* CONFIG_DEBUG_PAGEALLOC */
/* create bolted the linear mapping in the hash table */
for_each_mem_range(i, &base, &end) {
@@ -1935,6 +1928,8 @@ long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
}
#ifdef CONFIG_DEBUG_PAGEALLOC
+static DEFINE_SPINLOCK(linear_map_hash_lock);
+
static void kernel_map_linear_page(unsigned long vaddr, unsigned long lmi)
{
unsigned long hash;
--
2.25.0
^ permalink raw reply related
* [PATCH v2 1/4] powerpc: Enable KFENCE for PPC32
From: Christophe Leroy @ 2021-03-04 14:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Alexander Potapenko, Marco Elver, Dmitry Vyukov
Cc: linuxppc-dev, linux-kernel, kasan-dev
Add architecture specific implementation details for KFENCE and enable
KFENCE for the ppc32 architecture. In particular, this implements the
required interface in <asm/kfence.h>.
KFENCE requires that attributes for pages from its memory pool can
individually be set. Therefore, force the Read/Write linear map to be
mapped at page granularity.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Marco Elver <elver@google.com>
---
v2: Added debug_pagealloc_enabled_or_kfence()
---
arch/powerpc/Kconfig | 13 ++++++------
arch/powerpc/include/asm/kfence.h | 33 +++++++++++++++++++++++++++++++
arch/powerpc/mm/book3s32/mmu.c | 2 +-
arch/powerpc/mm/fault.c | 7 ++++++-
arch/powerpc/mm/init_32.c | 3 +++
arch/powerpc/mm/mmu_decl.h | 5 +++++
arch/powerpc/mm/nohash/8xx.c | 4 ++--
7 files changed, 57 insertions(+), 10 deletions(-)
create mode 100644 arch/powerpc/include/asm/kfence.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 386ae12d8523..d46db0bfb998 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -185,6 +185,7 @@ config PPC
select HAVE_ARCH_KASAN if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KASAN_VMALLOC if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KGDB
+ select HAVE_ARCH_KFENCE if PPC32
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_NVRAM_OPS
@@ -786,7 +787,7 @@ config THREAD_SHIFT
config DATA_SHIFT_BOOL
bool "Set custom data alignment"
depends on ADVANCED_OPTIONS
- depends on STRICT_KERNEL_RWX || DEBUG_PAGEALLOC
+ depends on STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE
depends on PPC_BOOK3S_32 || (PPC_8xx && !PIN_TLB_DATA && !STRICT_KERNEL_RWX)
help
This option allows you to set the kernel data alignment. When
@@ -798,13 +799,13 @@ config DATA_SHIFT_BOOL
config DATA_SHIFT
int "Data shift" if DATA_SHIFT_BOOL
default 24 if STRICT_KERNEL_RWX && PPC64
- range 17 28 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC) && PPC_BOOK3S_32
- range 19 23 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC) && PPC_8xx
+ range 17 28 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE) && PPC_BOOK3S_32
+ range 19 23 if (STRICT_KERNEL_RWX || DEBUG_PAGEALLOC || KFENCE) && PPC_8xx
default 22 if STRICT_KERNEL_RWX && PPC_BOOK3S_32
- default 18 if DEBUG_PAGEALLOC && PPC_BOOK3S_32
+ default 18 if (DEBUG_PAGEALLOC || KFENCE) && PPC_BOOK3S_32
default 23 if STRICT_KERNEL_RWX && PPC_8xx
- default 23 if DEBUG_PAGEALLOC && PPC_8xx && PIN_TLB_DATA
- default 19 if DEBUG_PAGEALLOC && PPC_8xx
+ default 23 if (DEBUG_PAGEALLOC || KFENCE) && PPC_8xx && PIN_TLB_DATA
+ default 19 if (DEBUG_PAGEALLOC || KFENCE) && PPC_8xx
default PPC_PAGE_SHIFT
help
On Book3S 32 (603+), DBATs are used to map kernel text and rodata RO.
diff --git a/arch/powerpc/include/asm/kfence.h b/arch/powerpc/include/asm/kfence.h
new file mode 100644
index 000000000000..a9846b68c6b9
--- /dev/null
+++ b/arch/powerpc/include/asm/kfence.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * powerpc KFENCE support.
+ *
+ * Copyright (C) 2020 CS GROUP France
+ */
+
+#ifndef __ASM_POWERPC_KFENCE_H
+#define __ASM_POWERPC_KFENCE_H
+
+#include <linux/mm.h>
+#include <asm/pgtable.h>
+
+static inline bool arch_kfence_init_pool(void)
+{
+ return true;
+}
+
+static inline bool kfence_protect_page(unsigned long addr, bool protect)
+{
+ pte_t *kpte = virt_to_kpte(addr);
+
+ if (protect) {
+ pte_update(&init_mm, addr, kpte, _PAGE_PRESENT, 0, 0);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+ } else {
+ pte_update(&init_mm, addr, kpte, 0, _PAGE_PRESENT, 0);
+ }
+
+ return true;
+}
+
+#endif /* __ASM_POWERPC_KFENCE_H */
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index d7eb266a3f7a..a0db398b5c26 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -162,7 +162,7 @@ unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
unsigned long border = (unsigned long)__init_begin - PAGE_OFFSET;
- if (debug_pagealloc_enabled() || __map_without_bats) {
+ if (debug_pagealloc_enabled_or_kfence() || __map_without_bats) {
pr_debug_once("Read-Write memory mapped without BATs\n");
if (base >= border)
return base;
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index bb368257b55c..bea13682c909 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -32,6 +32,7 @@
#include <linux/context_tracking.h>
#include <linux/hugetlb.h>
#include <linux/uaccess.h>
+#include <linux/kfence.h>
#include <asm/firmware.h>
#include <asm/interrupt.h>
@@ -418,8 +419,12 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
* take a page fault to a kernel address or a page fault to a user
* address outside of dedicated places
*/
- if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write)))
+ if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write))) {
+ if (kfence_handle_page_fault(address, is_write, regs))
+ return 0;
+
return SIGSEGV;
+ }
/*
* If we're in an interrupt, have no user context or are running
diff --git a/arch/powerpc/mm/init_32.c b/arch/powerpc/mm/init_32.c
index 02c7db4087cb..3d690be48e84 100644
--- a/arch/powerpc/mm/init_32.c
+++ b/arch/powerpc/mm/init_32.c
@@ -97,6 +97,9 @@ static void __init MMU_setup(void)
if (IS_ENABLED(CONFIG_PPC_8xx))
return;
+ if (IS_ENABLED(CONFIG_KFENCE))
+ __map_without_ltlbs = 1;
+
if (debug_pagealloc_enabled())
__map_without_ltlbs = 1;
diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
index 998810e68562..7dac910c0b21 100644
--- a/arch/powerpc/mm/mmu_decl.h
+++ b/arch/powerpc/mm/mmu_decl.h
@@ -185,3 +185,8 @@ void ptdump_check_wx(void);
#else
static inline void ptdump_check_wx(void) { }
#endif
+
+static inline bool debug_pagealloc_enabled_or_kfence(void)
+{
+ return IS_ENABLED(CONFIG_KFENCE) || debug_pagealloc_enabled();
+}
diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c
index 19a3eec1d8c5..71bfdbedacee 100644
--- a/arch/powerpc/mm/nohash/8xx.c
+++ b/arch/powerpc/mm/nohash/8xx.c
@@ -149,7 +149,7 @@ unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
unsigned long etext8 = ALIGN(__pa(_etext), SZ_8M);
unsigned long sinittext = __pa(_sinittext);
- bool strict_boundary = strict_kernel_rwx_enabled() || debug_pagealloc_enabled();
+ bool strict_boundary = strict_kernel_rwx_enabled() || debug_pagealloc_enabled_or_kfence();
unsigned long boundary = strict_boundary ? sinittext : etext8;
unsigned long einittext8 = ALIGN(__pa(_einittext), SZ_8M);
@@ -161,7 +161,7 @@ unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
return 0;
mmu_mapin_ram_chunk(0, boundary, PAGE_KERNEL_TEXT, true);
- if (debug_pagealloc_enabled()) {
+ if (debug_pagealloc_enabled_or_kfence()) {
top = boundary;
} else {
mmu_mapin_ram_chunk(boundary, einittext8, PAGE_KERNEL_TEXT, true);
--
2.25.0
^ permalink raw reply related
* [PATCH v2 4/4] powerpc: Enable KFENCE on BOOK3S/64
From: Christophe Leroy @ 2021-03-04 14:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Alexander Potapenko, Marco Elver, Dmitry Vyukov
Cc: linuxppc-dev, linux-kernel, kasan-dev
In-Reply-To: <8dfe1bd2abde26337c1d8c1ad0acfcc82185e0d5.1614868445.git.christophe.leroy@csgroup.eu>
This reuses the DEBUG_PAGEALLOC logic.
Tested on qemu with ppc64_defconfig + CONFIG_KFENCE + CONFIG_KUNIT +
CONFIG_KFENCE_KUNIT_TEST.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/kfence.h | 8 ++++++++
arch/powerpc/mm/book3s64/hash_utils.c | 29 +++++++++++++++++++++------
3 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index d46db0bfb998..67c47b60cc84 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -185,7 +185,7 @@ config PPC
select HAVE_ARCH_KASAN if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KASAN_VMALLOC if PPC32 && PPC_PAGE_SHIFT <= 14
select HAVE_ARCH_KGDB
- select HAVE_ARCH_KFENCE if PPC32
+ select HAVE_ARCH_KFENCE if ARCH_SUPPORTS_DEBUG_PAGEALLOC
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_NVRAM_OPS
diff --git a/arch/powerpc/include/asm/kfence.h b/arch/powerpc/include/asm/kfence.h
index a9846b68c6b9..532cc1a92fa5 100644
--- a/arch/powerpc/include/asm/kfence.h
+++ b/arch/powerpc/include/asm/kfence.h
@@ -11,11 +11,18 @@
#include <linux/mm.h>
#include <asm/pgtable.h>
+#if defined(CONFIG_PPC64) && !defined(PPC64_ELF_ABI_v2)
+#define ARCH_FUNC_PREFIX "."
+#endif
+
static inline bool arch_kfence_init_pool(void)
{
return true;
}
+#ifdef CONFIG_PPC64
+bool kfence_protect_page(unsigned long addr, bool protect);
+#else
static inline bool kfence_protect_page(unsigned long addr, bool protect)
{
pte_t *kpte = virt_to_kpte(addr);
@@ -29,5 +36,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect)
return true;
}
+#endif
#endif /* __ASM_POWERPC_KFENCE_H */
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index cb09a49be798..b967a6403e59 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -323,8 +323,8 @@ int htab_bolt_mapping(unsigned long vstart, unsigned long vend,
break;
cond_resched();
- if (debug_pagealloc_enabled() &&
- (paddr >> PAGE_SHIFT) < linear_map_hash_count)
+ if (debug_pagealloc_enabled_or_kfence() &&
+ (paddr >> PAGE_SHIFT) < linear_map_hash_count)
linear_map_hash_slots[paddr >> PAGE_SHIFT] = ret | 0x80;
}
return ret < 0 ? ret : 0;
@@ -661,7 +661,7 @@ static void __init htab_init_page_sizes(void)
bool aligned = true;
init_hpte_page_sizes();
- if (!debug_pagealloc_enabled()) {
+ if (!debug_pagealloc_enabled_or_kfence()) {
/*
* Pick a size for the linear mapping. Currently, we only
* support 16M, 1M and 4K which is the default
@@ -949,7 +949,7 @@ static void __init htab_initialize(void)
prot = pgprot_val(PAGE_KERNEL);
- if (debug_pagealloc_enabled()) {
+ if (debug_pagealloc_enabled_or_kfence()) {
linear_map_hash_count = memblock_end_of_DRAM() >> PAGE_SHIFT;
linear_map_hash_slots = memblock_alloc_try_nid(
linear_map_hash_count, 1, MEMBLOCK_LOW_LIMIT,
@@ -1927,7 +1927,7 @@ long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
return slot;
}
-#ifdef CONFIG_DEBUG_PAGEALLOC
+#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
static DEFINE_SPINLOCK(linear_map_hash_lock);
static void kernel_map_linear_page(unsigned long vaddr, unsigned long lmi)
@@ -1982,6 +1982,21 @@ static void kernel_unmap_linear_page(unsigned long vaddr, unsigned long lmi)
mmu_kernel_ssize, 0);
}
+#ifdef CONFIG_KFENCE
+bool kfence_protect_page(unsigned long addr, bool protect)
+{
+ unsigned long lmi = __pa(addr) >> PAGE_SHIFT;
+
+ if (protect)
+ kernel_unmap_linear_page(addr, lmi);
+ else
+ kernel_map_linear_page(addr, lmi);
+
+ return true;
+}
+#endif
+
+#ifdef CONFIG_DEBUG_PAGEALLOC
void __kernel_map_pages(struct page *page, int numpages, int enable)
{
unsigned long flags, vaddr, lmi;
@@ -2000,7 +2015,9 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
}
local_irq_restore(flags);
}
-#endif /* CONFIG_DEBUG_PAGEALLOC */
+#endif
+
+#endif /* CONFIG_DEBUG_PAGEALLOC || CONFIG_KFENCE */
void hash__setup_initial_memory_limit(phys_addr_t first_memblock_base,
phys_addr_t first_memblock_size)
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Mark Rutland @ 2021-03-04 14:57 UTC (permalink / raw)
To: Marco Elver
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linuxppc-dev, linux-arm-kernel
In-Reply-To: <YD+o5QkCZN97mH8/@elver.google.com>
[adding Mark Brown]
On Wed, Mar 03, 2021 at 04:20:43PM +0100, Marco Elver wrote:
> 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.
I agree this behaviour isn't desireable, and that the lack of
documentation is unfortunate.
It looks like GCC is happy to give us the function-entry-time FP if we use
__builtin_frame_address(1), and assuming clang is similarly happy we can do:
| diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
| index ad20981dfda4..5dfbf915eb7f 100644
| --- a/arch/arm64/kernel/stacktrace.c
| +++ b/arch/arm64/kernel/stacktrace.c
| @@ -203,8 +203,8 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
| 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);
| + (unsigned long)__builtin_frame_address(1),
| + (unsigned long)__builtin_return_address(0));
| else
| start_backtrace(&frame, thread_saved_fp(task),
| thread_saved_pc(task));
... such that arch_stack_walk() will try to avoid including itself in a
trace, and so the existing skipping should (w/ caveats below) skip
stack_trace_save() or stack_trace_save_tsk().
If that works for you, I can spin that as a patch, though we'll need to
check that doesn't introduce a new fencepost error elsewhere.
The bigger problem here is that skipping is dodgy to begin with, and
this is still liable to break in some cases. One big concern is that
(especially with LTO) we cannot guarantee the compiler will not inline
or outline functions, causing the skipp value to be too large or too
small. That's liable to happen to callers, and in theory (though
unlikely in practice), portions of arch_stack_walk() or
stack_trace_save() could get outlined too.
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.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v2] powerpc/32: remove bogus ppc_select syscall
From: Arnd Bergmann @ 2021-03-04 15:17 UTC (permalink / raw)
To: Christophe Leroy
Cc: linuxppc-dev, linux-kernel@vger.kernel.org, Paul Mackerras,
halesh.sadashiv
In-Reply-To: <2daa39fa210d971863a6f9ac7c81849764e7a261.1614862233.git.christophe.leroy@csgroup.eu>
On Thu, Mar 4, 2021 at 1:51 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The ppc_select function was introduced in linux-2.3.48 in order to support
> code confusing the legacy select() calling convention with the standard one.
> Even 24 years ago, all correctly built code should not have done this and
> could have easily been phased out. Nothing that was compiled later should
> actually try to use the old_select interface, and it would have been broken
> already on all ppc64 kernels with the syscall emulation layer.
>
> This patch brings the 32 bit compat ABI and the native 32 bit ABI for
> powerpc into a consistent state, by removing support for both the
> old_select system call number and the handler for it.
>
> The bug report triggering this came from
> Halesh Sadashiiv <halesh.sadashiv@ap.sony.com>, who discovered that the
> 32 bit implementation of ppc_select would in case of a negative number
> of file descriptors incorrectly return -EFAULT instead of -EINVAL.
> There seems to be no way to fix this problem in a way that would
> keep broken pre-1997 binaries running.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Halesh Sadashiiv <halesh.sadashiv@ap.sony.com>
> [chleroy: Rebased and updated the number of years elapsed in the commit message]
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> First version was in 2008, at that time it was rejected, see
> http://patchwork.ozlabs.org/project/linuxppc-dev/patch/200809240839.14902.arnd@arndb.de/
The patch from 2008 did two things:
- it removed the ppc32 specific 'select' syscall at #82
- it fixed the generic '_newselect' syscall at #142
Back then, the decision was to only address the second issue, which
got merged in commit dad2f2fb0fc7 ("powerpc: Fix wrong error code from
ppc32 select syscall").
It is probably ok to remove the old select system call now, but
my changelog text no longer makes sense, as the patch has nothing
to do with the bug that was reported back then.
Arnd
^ permalink raw reply
* Re: [PATCH v2] powerpc/32: remove bogus ppc_select syscall
From: Christophe Leroy @ 2021-03-04 15:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linuxppc-dev, linux-kernel@vger.kernel.org, Paul Mackerras,
halesh.sadashiv
In-Reply-To: <CAK8P3a2=qOG1iLhw2fi=r128bRMdfNx4BseXONiS7vrnbVvr6w@mail.gmail.com>
Le 04/03/2021 à 16:17, Arnd Bergmann a écrit :
> On Thu, Mar 4, 2021 at 1:51 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The ppc_select function was introduced in linux-2.3.48 in order to support
>> code confusing the legacy select() calling convention with the standard one.
>> Even 24 years ago, all correctly built code should not have done this and
>> could have easily been phased out. Nothing that was compiled later should
>> actually try to use the old_select interface, and it would have been broken
>> already on all ppc64 kernels with the syscall emulation layer.
>>
>> This patch brings the 32 bit compat ABI and the native 32 bit ABI for
>> powerpc into a consistent state, by removing support for both the
>> old_select system call number and the handler for it.
>>
>> The bug report triggering this came from
>> Halesh Sadashiiv <halesh.sadashiv@ap.sony.com>, who discovered that the
>> 32 bit implementation of ppc_select would in case of a negative number
>> of file descriptors incorrectly return -EFAULT instead of -EINVAL.
>> There seems to be no way to fix this problem in a way that would
>> keep broken pre-1997 binaries running.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: Halesh Sadashiiv <halesh.sadashiv@ap.sony.com>
>> [chleroy: Rebased and updated the number of years elapsed in the commit message]
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> First version was in 2008, at that time it was rejected, see
>> http://patchwork.ozlabs.org/project/linuxppc-dev/patch/200809240839.14902.arnd@arndb.de/
>
> The patch from 2008 did two things:
>
> - it removed the ppc32 specific 'select' syscall at #82
> - it fixed the generic '_newselect' syscall at #142
>
> Back then, the decision was to only address the second issue, which
> got merged in commit dad2f2fb0fc7 ("powerpc: Fix wrong error code from
> ppc32 select syscall").
>
> It is probably ok to remove the old select system call now, but
> my changelog text no longer makes sense, as the patch has nothing
> to do with the bug that was reported back then.
>
I understood that the original reported bug was that calling that version of select() with a
negative value as first parametre would lead to a -EFAULT instead of -EINVAL. That's exactly the
case here, if you set n = -1 you get into this (unsigned long)n > 4096 then the buffer is at
0xffffffff and access_ok() won't grand access to it so the return value will be -EFAULT instead of
-EINVAL.
Am I missing something ?
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-04 15:30 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linuxppc-dev, Linux ARM
In-Reply-To: <20210304145730.GC54534@C02TD0UTHF1T.local>
On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> [adding Mark Brown]
>
> On Wed, Mar 03, 2021 at 04:20:43PM +0100, Marco Elver wrote:
> > 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.
>
> I agree this behaviour isn't desireable, and that the lack of
> documentation is unfortunate.
>
> It looks like GCC is happy to give us the function-entry-time FP if we use
> __builtin_frame_address(1), and assuming clang is similarly happy we can do:
>
> | diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> | index ad20981dfda4..5dfbf915eb7f 100644
> | --- a/arch/arm64/kernel/stacktrace.c
> | +++ b/arch/arm64/kernel/stacktrace.c
> | @@ -203,8 +203,8 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> | 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);
> | + (unsigned long)__builtin_frame_address(1),
> | + (unsigned long)__builtin_return_address(0));
> | else
> | start_backtrace(&frame, thread_saved_fp(task),
> | thread_saved_pc(task));
>
> ... such that arch_stack_walk() will try to avoid including itself in a
> trace, and so the existing skipping should (w/ caveats below) skip
> stack_trace_save() or stack_trace_save_tsk().
Thank you! Yes, that works.
> If that works for you, I can spin that as a patch, though we'll need to
> check that doesn't introduce a new fencepost error elsewhere.
>
> The bigger problem here is that skipping is dodgy to begin with, and
> this is still liable to break in some cases. One big concern is that
> (especially with LTO) we cannot guarantee the compiler will not inline
> or outline functions, causing the skipp value to be too large or too
> small. That's liable to happen to callers, and in theory (though
> unlikely in practice), portions of arch_stack_walk() or
> stack_trace_save() could get outlined too.
>
> 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.
Will LTO and friends respect 'noinline'? One thing I also noticed is
that tail calls would also cause the stack trace to appear somewhat
incomplete (for some of my tests I've disabled tail call
optimizations). Is there a way to also mark a function
non-tail-callable? But I'm also not sure if with all that we'd be
guaranteed the code we want, even though in practice it might.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH 14/17] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Robin Murphy @ 2021-03-04 15:25 UTC (permalink / raw)
To: Christoph Hellwig, Joerg Roedel, Will Deacon, Li Yang
Cc: kvm, linux-arm-msm, linuxppc-dev, dri-devel, virtualization,
iommu, netdev, freedreno, David Woodhouse, linux-arm-kernel
In-Reply-To: <20210301084257.945454-15-hch@lst.de>
On 2021-03-01 08:42, Christoph Hellwig wrote:
> Use explicit methods for setting and querying the information instead.
Now that everyone's using iommu-dma, is there any point in bouncing this
through the drivers at all? Seems like it would make more sense for the
x86 drivers to reflect their private options back to iommu_dma_strict
(and allow Intel's caching mode to override it as well), then have
iommu_dma_init_domain just test !iommu_dma_strict &&
domain->ops->flush_iotlb_all.
Robin.
> Also remove the now unused iommu_domain_get_attr functionality.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/iommu/amd/iommu.c | 23 ++-------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 47 ++++++-----------
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 56 +++++----------------
> drivers/iommu/dma-iommu.c | 8 ++-
> drivers/iommu/intel/iommu.c | 27 ++--------
> drivers/iommu/iommu.c | 19 +++----
> include/linux/iommu.h | 17 ++-----
> 7 files changed, 51 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a69a8b573e40d0..37a8e51db17656 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1771,24 +1771,11 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
> return acpihid_device_group(dev);
> }
>
> -static int amd_iommu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> +static bool amd_iommu_dma_use_flush_queue(struct iommu_domain *domain)
> {
> - switch (domain->type) {
> - case IOMMU_DOMAIN_UNMANAGED:
> - return -ENODEV;
> - case IOMMU_DOMAIN_DMA:
> - switch (attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - *(int *)data = !amd_iommu_unmap_flush;
> - return 0;
> - default:
> - return -ENODEV;
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> + if (domain->type != IOMMU_DOMAIN_DMA)
> + return false;
> + return !amd_iommu_unmap_flush;
> }
>
> /*****************************************************************************
> @@ -2257,7 +2244,7 @@ const struct iommu_ops amd_iommu_ops = {
> .release_device = amd_iommu_release_device,
> .probe_finalize = amd_iommu_probe_finalize,
> .device_group = amd_iommu_device_group,
> - .domain_get_attr = amd_iommu_domain_get_attr,
> + .dma_use_flush_queue = amd_iommu_dma_use_flush_queue,
> .get_resv_regions = amd_iommu_get_resv_regions,
> .put_resv_regions = generic_iommu_put_resv_regions,
> .is_attach_deferred = amd_iommu_is_attach_deferred,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 8594b4a8304375..bf96172e8c1f71 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2449,33 +2449,21 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> return group;
> }
>
> -static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> +static bool arm_smmu_dma_use_flush_queue(struct iommu_domain *domain)
> {
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>
> - switch (domain->type) {
> - case IOMMU_DOMAIN_UNMANAGED:
> - switch (attr) {
> - case DOMAIN_ATTR_NESTING:
> - *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> - return 0;
> - default:
> - return -ENODEV;
> - }
> - break;
> - case IOMMU_DOMAIN_DMA:
> - switch (attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - *(int *)data = smmu_domain->non_strict;
> - return 0;
> - default:
> - return -ENODEV;
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> + if (domain->type != IOMMU_DOMAIN_DMA)
> + return false;
> + return smmu_domain->non_strict;
> +}
> +
> +
> +static void arm_smmu_dma_enable_flush_queue(struct iommu_domain *domain)
> +{
> + if (domain->type != IOMMU_DOMAIN_DMA)
> + return;
> + to_smmu_domain(domain)->non_strict = true;
> }
>
> static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> @@ -2505,13 +2493,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> }
> break;
> case IOMMU_DOMAIN_DMA:
> - switch(attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - smmu_domain->non_strict = *(int *)data;
> - break;
> - default:
> - ret = -ENODEV;
> - }
> + ret = -ENODEV;
> break;
> default:
> ret = -EINVAL;
> @@ -2619,7 +2601,8 @@ static struct iommu_ops arm_smmu_ops = {
> .probe_device = arm_smmu_probe_device,
> .release_device = arm_smmu_release_device,
> .device_group = arm_smmu_device_group,
> - .domain_get_attr = arm_smmu_domain_get_attr,
> + .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,
> .of_xlate = arm_smmu_of_xlate,
> .get_resv_regions = arm_smmu_get_resv_regions,
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d8c6bfde6a6158..e7893e96f5177a 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1481,42 +1481,20 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
> return group;
> }
>
> -static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> +static bool arm_smmu_dma_use_flush_queue(struct iommu_domain *domain)
> {
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>
> - switch(domain->type) {
> - case IOMMU_DOMAIN_UNMANAGED:
> - switch (attr) {
> - case DOMAIN_ATTR_NESTING:
> - *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> - return 0;
> - case DOMAIN_ATTR_IO_PGTABLE_CFG: {
> - struct io_pgtable_domain_attr *pgtbl_cfg = data;
> - *pgtbl_cfg = smmu_domain->pgtbl_cfg;
> + if (domain->type != IOMMU_DOMAIN_DMA)
> + return false;
> + return smmu_domain->pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT;
> +}
>
> - return 0;
> - }
> - default:
> - return -ENODEV;
> - }
> - break;
> - case IOMMU_DOMAIN_DMA:
> - switch (attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE: {
> - bool non_strict = smmu_domain->pgtbl_cfg.quirks &
> - IO_PGTABLE_QUIRK_NON_STRICT;
> - *(int *)data = non_strict;
> - return 0;
> - }
> - default:
> - return -ENODEV;
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> +static void arm_smmu_dma_enable_flush_queue(struct iommu_domain *domain)
> +{
> + if (domain->type != IOMMU_DOMAIN_DMA)
> + return;
> + to_smmu_domain(domain)->pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> }
>
> static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> @@ -1557,16 +1535,7 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> }
> break;
> case IOMMU_DOMAIN_DMA:
> - switch (attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - if (*(int *)data)
> - smmu_domain->pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> - else
> - smmu_domain->pgtbl_cfg.quirks &= ~IO_PGTABLE_QUIRK_NON_STRICT;
> - break;
> - default:
> - ret = -ENODEV;
> - }
> + ret = -ENODEV;
> break;
> default:
> ret = -EINVAL;
> @@ -1631,7 +1600,8 @@ static struct iommu_ops arm_smmu_ops = {
> .probe_device = arm_smmu_probe_device,
> .release_device = arm_smmu_release_device,
> .device_group = arm_smmu_device_group,
> - .domain_get_attr = arm_smmu_domain_get_attr,
> + .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,
> .of_xlate = arm_smmu_of_xlate,
> .get_resv_regions = arm_smmu_get_resv_regions,
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9ab6ee22c11088..d3fe5aad9d6ecf 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -305,8 +305,8 @@ static void iommu_dma_flush_iotlb_all(struct iova_domain *iovad)
> cookie = container_of(iovad, struct iommu_dma_cookie, iovad);
> domain = cookie->fq_domain;
> /*
> - * The IOMMU driver supporting DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
> - * implies that ops->flush_iotlb_all must be non-NULL.
> + * The IOMMU driver supporting a DMA flush queue implies that
> + * ops->flush_iotlb_all must be non-NULL.
> */
> domain->ops->flush_iotlb_all(domain);
> }
> @@ -329,7 +329,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> unsigned long order, base_pfn;
> struct iova_domain *iovad;
> - int attr;
>
> if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE)
> return -EINVAL;
> @@ -365,8 +364,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>
> init_iova_domain(iovad, 1UL << order, base_pfn);
>
> - if (!cookie->fq_domain && !iommu_domain_get_attr(domain,
> - DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, &attr) && attr) {
> + if (!cookie->fq_domain && iommu_dma_use_flush_queue(domain)) {
> if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> iommu_dma_entry_dtor))
> pr_warn("iova flush queue initialization failed\n");
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index ee0932307d646b..eaa80c33f4bc91 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5453,13 +5453,13 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
> return ret;
> }
>
> -static bool domain_use_flush_queue(void)
> +static bool intel_iommu_dma_use_flush_queue(struct iommu_domain *domain)
> {
> struct dmar_drhd_unit *drhd;
> struct intel_iommu *iommu;
> bool r = true;
>
> - if (intel_iommu_strict)
> + if (domain->type != IOMMU_DOMAIN_DMA || intel_iommu_strict)
> return false;
>
> /*
> @@ -5483,27 +5483,6 @@ static bool domain_use_flush_queue(void)
> return r;
> }
>
> -static int
> -intel_iommu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> -{
> - switch (domain->type) {
> - case IOMMU_DOMAIN_UNMANAGED:
> - return -ENODEV;
> - case IOMMU_DOMAIN_DMA:
> - switch (attr) {
> - case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
> - *(int *)data = domain_use_flush_queue();
> - return 0;
> - default:
> - return -ENODEV;
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> -}
> -
> /*
> * Check that the device does not live on an external facing PCI port that is
> * marked as untrusted. Such devices should not be able to apply quirks and
> @@ -5576,7 +5555,7 @@ const struct iommu_ops intel_iommu_ops = {
> .capable = intel_iommu_capable,
> .domain_alloc = intel_iommu_domain_alloc,
> .domain_free = intel_iommu_domain_free,
> - .domain_get_attr = intel_iommu_domain_get_attr,
> + .dma_use_flush_queue = intel_iommu_dma_use_flush_queue,
> .domain_set_attr = intel_iommu_domain_set_attr,
> .attach_dev = intel_iommu_attach_device,
> .detach_dev = intel_iommu_detach_device,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 23daaea7883b75..0f12c4d58cdc42 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1512,12 +1512,8 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
> if (!group->domain)
> group->domain = dom;
>
> - if (!iommu_dma_strict) {
> - int attr = 1;
> - iommu_domain_set_attr(dom,
> - DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
> - &attr);
> - }
> + if (!iommu_dma_strict && dom->ops->dma_enable_flush_queue)
> + dom->ops->dma_enable_flush_queue(dom);
>
> return 0;
> }
> @@ -2664,14 +2660,13 @@ static int __init iommu_init(void)
> }
> core_initcall(iommu_init);
>
> -int iommu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> +bool iommu_dma_use_flush_queue(struct iommu_domain *domain)
> {
> - if (!domain->ops->domain_get_attr)
> - return -EINVAL;
> - return domain->ops->domain_get_attr(domain, attr, data);
> + if (!domain->ops->dma_use_flush_queue)
> + return false;
> + return domain->ops->dma_use_flush_queue(domain);
> }
> -EXPORT_SYMBOL_GPL(iommu_domain_get_attr);
> +EXPORT_SYMBOL_GPL(iommu_dma_use_flush_queue);
>
> int iommu_domain_set_attr(struct iommu_domain *domain,
> enum iommu_attr attr, void *data)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c15a8658daad64..f30de33c6ff56e 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -108,7 +108,6 @@ enum iommu_cap {
>
> enum iommu_attr {
> DOMAIN_ATTR_NESTING, /* two stages of translation */
> - DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
> DOMAIN_ATTR_IO_PGTABLE_CFG,
> DOMAIN_ATTR_MAX,
> };
> @@ -194,7 +193,8 @@ struct iommu_iotlb_gather {
> * @probe_finalize: Do final setup work after the device is added to an IOMMU
> * group and attached to the groups domain
> * @device_group: find iommu group for a particular device
> - * @domain_get_attr: Query domain attributes
> + * @dma_use_flush_queue: Returns %true if a DMA flush queue is used
> + * @dma_enable_flush_queue: Try to enable the DMA flush queue
> * @domain_set_attr: Change domain attributes
> * @get_resv_regions: Request list of reserved regions for a device
> * @put_resv_regions: Free list of reserved regions for a device
> @@ -244,8 +244,8 @@ struct iommu_ops {
> void (*release_device)(struct device *dev);
> void (*probe_finalize)(struct device *dev);
> struct iommu_group *(*device_group)(struct device *dev);
> - int (*domain_get_attr)(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data);
> + bool (*dma_use_flush_queue)(struct iommu_domain *domain);
> + void (*dma_enable_flush_queue)(struct iommu_domain *domain);
> int (*domain_set_attr)(struct iommu_domain *domain,
> enum iommu_attr attr, void *data);
>
> @@ -491,8 +491,7 @@ extern int iommu_page_response(struct device *dev,
> extern int iommu_group_id(struct iommu_group *group);
> extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
>
> -extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
> - void *data);
> +bool iommu_dma_use_flush_queue(struct iommu_domain *domain);
> extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
> void *data);
>
> @@ -861,12 +860,6 @@ static inline int iommu_group_id(struct iommu_group *group)
> return -ENODEV;
> }
>
> -static inline int iommu_domain_get_attr(struct iommu_domain *domain,
> - enum iommu_attr attr, void *data)
> -{
> - return -EINVAL;
> -}
> -
> static inline int iommu_domain_set_attr(struct iommu_domain *domain,
> enum iommu_attr attr, void *data)
> {
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/32: remove bogus ppc_select syscall
From: Arnd Bergmann @ 2021-03-04 15:30 UTC (permalink / raw)
To: Christophe Leroy
Cc: linuxppc-dev, linux-kernel@vger.kernel.org, Paul Mackerras,
halesh.sadashiv
In-Reply-To: <aca143f6-fcfc-d89f-bb00-26e90257fbf6@csgroup.eu>
On Thu, Mar 4, 2021 at 4:24 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 04/03/2021 à 16:17, Arnd Bergmann a écrit :
> > On Thu, Mar 4, 2021 at 1:51 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> >> ---
> >> First version was in 2008, at that time it was rejected, see
> >> http://patchwork.ozlabs.org/project/linuxppc-dev/patch/200809240839.14902.arnd@arndb.de/
> >
> > The patch from 2008 did two things:
> >
> > - it removed the ppc32 specific 'select' syscall at #82
> > - it fixed the generic '_newselect' syscall at #142
> >
> > Back then, the decision was to only address the second issue, which
> > got merged in commit dad2f2fb0fc7 ("powerpc: Fix wrong error code from
> > ppc32 select syscall").
> >
> > It is probably ok to remove the old select system call now, but
> > my changelog text no longer makes sense, as the patch has nothing
> > to do with the bug that was reported back then.
> >
>
> I understood that the original reported bug was that calling that version of select() with a
> negative value as first parametre would lead to a -EFAULT instead of -EINVAL. That's exactly the
> case here, if you set n = -1 you get into this (unsigned long)n > 4096 then the buffer is at
> 0xffffffff and access_ok() won't grand access to it so the return value will be -EFAULT instead of
> -EINVAL.
>
> Am I missing something ?
This is the behavior of the ppc_select() implementation, but as far as
I can tell,
the bug report was for the problem that this behavior would happen for both
syscall #82 and syscall #142 when the correct behavior would have been to
only do it for #82 but not for #142.
Arnd
^ permalink raw reply
* Re: [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
From: Robin Murphy @ 2021-03-04 15:48 UTC (permalink / raw)
To: Christoph Hellwig, Joerg Roedel, Will Deacon, Li Yang
Cc: kvm, linux-arm-msm, linuxppc-dev, dri-devel, virtualization,
iommu, netdev, freedreno, David Woodhouse, linux-arm-kernel
In-Reply-To: <20210301084257.945454-17-hch@lst.de>
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.
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);
>
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc: sstep: Fix load and update emulation
From: Segher Boessenkool @ 2021-03-04 15:45 UTC (permalink / raw)
To: Naveen N. Rao
Cc: ravi.bangoria, ananth, jniethe5, paulus, Sandipan Das,
linuxppc-dev, dja
In-Reply-To: <20210303163127.GE1913@DESKTOP-TDPLP67.localdomain>
On Wed, Mar 03, 2021 at 10:01:27PM +0530, Naveen N. Rao wrote:
> On 2021/03/01 08:37PM, Segher Boessenkool wrote:
> > > And, r6 always ends up with 0xaea. It changes with the value I put into
> > > r6 though.
> >
> > That is exactly the behaviour specified for p8. 0aaa+0040=0aea.
> >
> > > Granted, this is all up in the air, but it does look like there is more
> > > going on and the value isn't the EA or the value at the address.
> >
> > That *is* the EA. The EA is the address the insn does the access at.
>
> I'm probably missing something here. 0xaaa is the value I stored at an
> offset of 64 bytes from the stack pointer (r1 is copied into r6). In the
> ldu instruction above, the EA is 64(r6), which should translate to
> r1+64. The data returned by the load would be 0xaaa, which should be
> discarded per the description you provided above. So, I would expect to
> see a 0xc0.. address in r6.
Yes, I misread your code it seems.
> In fact, this looks to be the behavior documented for P9:
>
> > > Power9 does:
> > >
> > > Load with Update Instructions (RA = 0)
> > > EA is placed into R0.
> > > Load with Update Instructions (RA = RT)
> > > The storage operand addressed by EA is accessed. The
> > > displacement
> > > field is added to the data returned by the load and placed into
> > > RT.
Yup. So on what cpu did you test?
Either way, the kernel should not emulate any particular cpu here, I'd
say, esp. since recent cpus do different things for this invalid form.
Segher
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Mark Rutland @ 2021-03-04 16:59 UTC (permalink / raw)
To: Marco Elver
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linuxppc-dev, Linux ARM
In-Reply-To: <CANpmjNOSpFbbDaH9hNucXrpzG=HpsoQpk5w-24x8sU_G-6cz0Q@mail.gmail.com>
On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > [adding Mark Brown]
> >
> > The bigger problem here is that skipping is dodgy to begin with, and
> > this is still liable to break in some cases. One big concern is that
> > (especially with LTO) we cannot guarantee the compiler will not inline
> > or outline functions, causing the skipp value to be too large or too
> > small. That's liable to happen to callers, and in theory (though
> > unlikely in practice), portions of arch_stack_walk() or
> > stack_trace_save() could get outlined too.
> >
> > 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.
>
> Will LTO and friends respect 'noinline'?
I hope so (and suspect we'd have more problems otherwise), but I don't
know whether they actually so.
I suspect even with 'noinline' the compiler is permitted to outline
portions of a function if it wanted to (and IIUC it could still make
specialized copies in the absence of 'noclone').
> One thing I also noticed is that tail calls would also cause the stack
> trace to appear somewhat incomplete (for some of my tests I've
> disabled tail call optimizations).
I assume you mean for a chain A->B->C where B tail-calls C, you get a
trace A->C? ... or is A going missing too?
> Is there a way to also mark a function non-tail-callable?
I think this can be bodged using __attribute__((optimize("$OPTIONS")))
on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
function-local optimization options), but I don't expect there's any way
to mark a callee as not being tail-callable.
Accoding to the GCC documentation, GCC won't TCO noreturn functions, but
obviously that's not something we can use generally.
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> But I'm also not sure if with all that we'd be guaranteed the code we
> want, even though in practice it might.
True! I'd just like to be on the least dodgy ground we can be.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-04 17:25 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linux-toolchains, linuxppc-dev, Linux ARM
In-Reply-To: <20210304165923.GA60457@C02TD0UTHF1T.local>
On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > [adding Mark Brown]
> > >
> > > The bigger problem here is that skipping is dodgy to begin with, and
> > > this is still liable to break in some cases. One big concern is that
> > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > or outline functions, causing the skipp value to be too large or too
> > > small. That's liable to happen to callers, and in theory (though
> > > unlikely in practice), portions of arch_stack_walk() or
> > > stack_trace_save() could get outlined too.
> > >
> > > 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.
> >
> > Will LTO and friends respect 'noinline'?
>
> I hope so (and suspect we'd have more problems otherwise), but I don't
> know whether they actually so.
>
> I suspect even with 'noinline' the compiler is permitted to outline
> portions of a function if it wanted to (and IIUC it could still make
> specialized copies in the absence of 'noclone').
>
> > One thing I also noticed is that tail calls would also cause the stack
> > trace to appear somewhat incomplete (for some of my tests I've
> > disabled tail call optimizations).
>
> I assume you mean for a chain A->B->C where B tail-calls C, you get a
> trace A->C? ... or is A going missing too?
Correct, it's just the A->C outcome.
> > Is there a way to also mark a function non-tail-callable?
>
> I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> function-local optimization options), but I don't expect there's any way
> to mark a callee as not being tail-callable.
I don't think this is reliable. It'd be
__attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
work if applied to the function we do not want to tail-call-optimize,
but would have to be applied to the function that does the tail-calling.
So it's a bit backwards, even if it worked.
> Accoding to the GCC documentation, GCC won't TCO noreturn functions, but
> obviously that's not something we can use generally.
>
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
Perhaps we can ask the toolchain folks to help add such an attribute. Or
maybe the feature already exists somewhere, but hidden.
+Cc linux-toolchains@vger.kernel.org
> > But I'm also not sure if with all that we'd be guaranteed the code we
> > want, even though in practice it might.
>
> True! I'd just like to be on the least dodgy ground we can be.
It's been dodgy for a while, and I'd welcome any low-cost fixes to make
it less dodgy in the short-term at least. :-)
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Nick Desaulniers @ 2021-03-04 17:54 UTC (permalink / raw)
To: Marco Elver
Cc: Mark Rutland, Catalin Marinas, Will Deacon, LKML, Mark Brown,
Paul Mackerras, kasan-dev, linux-toolchains, linuxppc-dev,
Linux ARM
In-Reply-To: <YEEYDSJeLPvqRAHZ@elver.google.com>
On Thu, Mar 4, 2021 at 9:42 AM Marco Elver <elver@google.com> wrote:
>
> On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> > On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > [adding Mark Brown]
> > > >
> > > > The bigger problem here is that skipping is dodgy to begin with, and
> > > > this is still liable to break in some cases. One big concern is that
> > > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > > or outline functions, causing the skipp value to be too large or too
> > > > small. That's liable to happen to callers, and in theory (though
> > > > unlikely in practice), portions of arch_stack_walk() or
> > > > stack_trace_save() could get outlined too.
> > > >
> > > > 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.
> > >
> > > Will LTO and friends respect 'noinline'?
> >
> > I hope so (and suspect we'd have more problems otherwise), but I don't
> > know whether they actually so.
> >
> > I suspect even with 'noinline' the compiler is permitted to outline
> > portions of a function if it wanted to (and IIUC it could still make
> > specialized copies in the absence of 'noclone').
> >
> > > One thing I also noticed is that tail calls would also cause the stack
> > > trace to appear somewhat incomplete (for some of my tests I've
> > > disabled tail call optimizations).
> >
> > I assume you mean for a chain A->B->C where B tail-calls C, you get a
> > trace A->C? ... or is A going missing too?
>
> Correct, it's just the A->C outcome.
>
> > > Is there a way to also mark a function non-tail-callable?
> >
> > I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> > on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> > function-local optimization options), but I don't expect there's any way
> > to mark a callee as not being tail-callable.
>
> I don't think this is reliable. It'd be
> __attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
> work if applied to the function we do not want to tail-call-optimize,
> but would have to be applied to the function that does the tail-calling.
> So it's a bit backwards, even if it worked.
>
> > Accoding to the GCC documentation, GCC won't TCO noreturn functions, but
> > obviously that's not something we can use generally.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
include/linux/compiler.h:246:
prevent_tail_call_optimization
commit a9a3ed1eff36 ("x86: Fix early boot crash on gcc-10, third try")
>
> Perhaps we can ask the toolchain folks to help add such an attribute. Or
> maybe the feature already exists somewhere, but hidden.
>
> +Cc linux-toolchains@vger.kernel.org
>
> > > But I'm also not sure if with all that we'd be guaranteed the code we
> > > want, even though in practice it might.
> >
> > True! I'd just like to be on the least dodgy ground we can be.
>
> It's been dodgy for a while, and I'd welcome any low-cost fixes to make
> it less dodgy in the short-term at least. :-)
>
> Thanks,
> -- Marco
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Mark Rutland @ 2021-03-04 18:01 UTC (permalink / raw)
To: Marco Elver
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linux-toolchains, linuxppc-dev, Linux ARM
In-Reply-To: <YEEYDSJeLPvqRAHZ@elver.google.com>
On Thu, Mar 04, 2021 at 06:25:33PM +0100, Marco Elver wrote:
> On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> > On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > [adding Mark Brown]
> > > >
> > > > The bigger problem here is that skipping is dodgy to begin with, and
> > > > this is still liable to break in some cases. One big concern is that
> > > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > > or outline functions, causing the skipp value to be too large or too
> > > > small. That's liable to happen to callers, and in theory (though
> > > > unlikely in practice), portions of arch_stack_walk() or
> > > > stack_trace_save() could get outlined too.
> > > >
> > > > 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.
> > >
> > > Will LTO and friends respect 'noinline'?
> >
> > I hope so (and suspect we'd have more problems otherwise), but I don't
> > know whether they actually so.
> >
> > I suspect even with 'noinline' the compiler is permitted to outline
> > portions of a function if it wanted to (and IIUC it could still make
> > specialized copies in the absence of 'noclone').
> >
> > > One thing I also noticed is that tail calls would also cause the stack
> > > trace to appear somewhat incomplete (for some of my tests I've
> > > disabled tail call optimizations).
> >
> > I assume you mean for a chain A->B->C where B tail-calls C, you get a
> > trace A->C? ... or is A going missing too?
>
> Correct, it's just the A->C outcome.
I'd assumed that those cases were benign, e.g. for livepatching what
matters is what can be returned to, so B disappearing from the trace
isn't a problem there.
Is the concern debugability, or is there a functional issue you have in
mind?
> > > Is there a way to also mark a function non-tail-callable?
> >
> > I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> > on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> > function-local optimization options), but I don't expect there's any way
> > to mark a callee as not being tail-callable.
>
> I don't think this is reliable. It'd be
> __attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
> work if applied to the function we do not want to tail-call-optimize,
> but would have to be applied to the function that does the tail-calling.
Yup; that's what I meant then I said you could do that on the caller but
not the callee.
I don't follow why you'd want to put this on the callee, though, so I
think I'm missing something. Considering a set of functions in different
compilation units:
A->B->C->D->E->F->G->H->I->J->K
... if K were marked in this way, and J was compiled with visibility of
this, J would stick around, but J's callers might not, and so the a
trace might see:
A->J->K
... do you just care about the final caller, i.e. you just need
certainty that J will be in the trace?
If so, we can somewhat bodge that by having K have an __always_inline
wrapper which has a barrier() or similar after the real call to K, so
the call couldn't be TCO'd.
Otherwise I'd expect we'd probably need to disable TCO generally.
> So it's a bit backwards, even if it worked.
>
> > Accoding to the GCC documentation, GCC won't TCO noreturn functions, but
> > obviously that's not something we can use generally.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>
> Perhaps we can ask the toolchain folks to help add such an attribute. Or
> maybe the feature already exists somewhere, but hidden.
>
> +Cc linux-toolchains@vger.kernel.org
>
> > > But I'm also not sure if with all that we'd be guaranteed the code we
> > > want, even though in practice it might.
> >
> > True! I'd just like to be on the least dodgy ground we can be.
>
> It's been dodgy for a while, and I'd welcome any low-cost fixes to make
> it less dodgy in the short-term at least. :-)
:)
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-04 18:22 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linux-toolchains, linuxppc-dev, Linux ARM
In-Reply-To: <20210304180154.GD60457@C02TD0UTHF1T.local>
On Thu, 4 Mar 2021 at 19:02, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Mar 04, 2021 at 06:25:33PM +0100, Marco Elver wrote:
> > On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> > > On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > > > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > [adding Mark Brown]
> > > > >
> > > > > The bigger problem here is that skipping is dodgy to begin with, and
> > > > > this is still liable to break in some cases. One big concern is that
> > > > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > > > or outline functions, causing the skipp value to be too large or too
> > > > > small. That's liable to happen to callers, and in theory (though
> > > > > unlikely in practice), portions of arch_stack_walk() or
> > > > > stack_trace_save() could get outlined too.
> > > > >
> > > > > 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.
> > > >
> > > > Will LTO and friends respect 'noinline'?
> > >
> > > I hope so (and suspect we'd have more problems otherwise), but I don't
> > > know whether they actually so.
> > >
> > > I suspect even with 'noinline' the compiler is permitted to outline
> > > portions of a function if it wanted to (and IIUC it could still make
> > > specialized copies in the absence of 'noclone').
> > >
> > > > One thing I also noticed is that tail calls would also cause the stack
> > > > trace to appear somewhat incomplete (for some of my tests I've
> > > > disabled tail call optimizations).
> > >
> > > I assume you mean for a chain A->B->C where B tail-calls C, you get a
> > > trace A->C? ... or is A going missing too?
> >
> > Correct, it's just the A->C outcome.
>
> I'd assumed that those cases were benign, e.g. for livepatching what
> matters is what can be returned to, so B disappearing from the trace
> isn't a problem there.
>
> Is the concern debugability, or is there a functional issue you have in
> mind?
For me, it's just been debuggability, and reliable test cases.
> > > > Is there a way to also mark a function non-tail-callable?
> > >
> > > I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> > > on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> > > function-local optimization options), but I don't expect there's any way
> > > to mark a callee as not being tail-callable.
> >
> > I don't think this is reliable. It'd be
> > __attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
> > work if applied to the function we do not want to tail-call-optimize,
> > but would have to be applied to the function that does the tail-calling.
>
> Yup; that's what I meant then I said you could do that on the caller but
> not the callee.
>
> I don't follow why you'd want to put this on the callee, though, so I
> think I'm missing something. Considering a set of functions in different
> compilation units:
>
> A->B->C->D->E->F->G->H->I->J->K
I was having this problem with KCSAN, where the compiler would
tail-call-optimize __tsan_X instrumentation. This would mean that
KCSAN runtime functions ended up in the trace, but the function where
the access happened would not. However, I don't care about the runtime
functions, and instead want to see the function where the access
happened. In that case, I'd like to just mark __tsan_X and any other
kcsan instrumentation functions as do-not-tail-call-optimize, which
would solve the problem.
The solution today is that when you compile a kernel with KCSAN, every
instrumented TU is compiled with -fno-optimize-sibling-calls. The
better solution would be to just mark KCSAN runtime functions somehow,
but permit tail calling other things. Although, I probably still want
to see the full trace, and would decide that having
-fno-optimize-sibling-calls is a small price to pay in a
debug-only-kernel to get complete traces.
> ... if K were marked in this way, and J was compiled with visibility of
> this, J would stick around, but J's callers might not, and so the a
> trace might see:
>
> A->J->K
>
> ... do you just care about the final caller, i.e. you just need
> certainty that J will be in the trace?
Yes. But maybe it's a special problem that only sanitizers have.
> If so, we can somewhat bodge that by having K have an __always_inline
> wrapper which has a barrier() or similar after the real call to K, so
> the call couldn't be TCO'd.
>
> Otherwise I'd expect we'd probably need to disable TCO generally.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
From: Tyrel Datwyler @ 2021-03-04 18:48 UTC (permalink / raw)
To: Jiapeng Chong
Cc: martin.petersen, linux-scsi, jejb, linux-kernel, paulus,
linuxppc-dev
In-Reply-To: <1614850124-54111-1-git-send-email-jiapeng.chong@linux.alibaba.com>
On 3/4/21 1:28 AM, Jiapeng Chong wrote:
> Fix the following coccicheck warnings:
>
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3483:60-61: WARNING opportunity for
> kobj_to_dev().
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> ---
> drivers/scsi/ibmvscsi/ibmvfc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 755313b..e5f1ca7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -3480,7 +3480,7 @@ static ssize_t ibmvfc_read_trace(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr,
> char *buf, loff_t off, size_t count)
> {
> - struct device *dev = container_of(kobj, struct device, kobj);
> + struct device *dev = kobj_to_dev(kobj);
> struct Scsi_Host *shost = class_to_shost(dev);
> struct ibmvfc_host *vhost = shost_priv(shost);
> unsigned long flags = 0;
>
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Mark Rutland @ 2021-03-04 18:51 UTC (permalink / raw)
To: Marco Elver
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linux-toolchains, linuxppc-dev, Linux ARM
In-Reply-To: <CANpmjNOZWuhqXATDjH3F=DMbpg2xOy0XppVJ+Wv2XjFh_crJJg@mail.gmail.com>
On Thu, Mar 04, 2021 at 07:22:53PM +0100, Marco Elver wrote:
> On Thu, 4 Mar 2021 at 19:02, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Mar 04, 2021 at 06:25:33PM +0100, Marco Elver wrote:
> > > On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> > > > On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > > > > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > > [adding Mark Brown]
> > > > > >
> > > > > > The bigger problem here is that skipping is dodgy to begin with, and
> > > > > > this is still liable to break in some cases. One big concern is that
> > > > > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > > > > or outline functions, causing the skipp value to be too large or too
> > > > > > small. That's liable to happen to callers, and in theory (though
> > > > > > unlikely in practice), portions of arch_stack_walk() or
> > > > > > stack_trace_save() could get outlined too.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Will LTO and friends respect 'noinline'?
> > > >
> > > > I hope so (and suspect we'd have more problems otherwise), but I don't
> > > > know whether they actually so.
> > > >
> > > > I suspect even with 'noinline' the compiler is permitted to outline
> > > > portions of a function if it wanted to (and IIUC it could still make
> > > > specialized copies in the absence of 'noclone').
> > > >
> > > > > One thing I also noticed is that tail calls would also cause the stack
> > > > > trace to appear somewhat incomplete (for some of my tests I've
> > > > > disabled tail call optimizations).
> > > >
> > > > I assume you mean for a chain A->B->C where B tail-calls C, you get a
> > > > trace A->C? ... or is A going missing too?
> > >
> > > Correct, it's just the A->C outcome.
> >
> > I'd assumed that those cases were benign, e.g. for livepatching what
> > matters is what can be returned to, so B disappearing from the trace
> > isn't a problem there.
> >
> > Is the concern debugability, or is there a functional issue you have in
> > mind?
>
> For me, it's just been debuggability, and reliable test cases.
>
> > > > > Is there a way to also mark a function non-tail-callable?
> > > >
> > > > I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> > > > on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> > > > function-local optimization options), but I don't expect there's any way
> > > > to mark a callee as not being tail-callable.
> > >
> > > I don't think this is reliable. It'd be
> > > __attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
> > > work if applied to the function we do not want to tail-call-optimize,
> > > but would have to be applied to the function that does the tail-calling.
> >
> > Yup; that's what I meant then I said you could do that on the caller but
> > not the callee.
> >
> > I don't follow why you'd want to put this on the callee, though, so I
> > think I'm missing something. Considering a set of functions in different
> > compilation units:
> >
> > A->B->C->D->E->F->G->H->I->J->K
>
> I was having this problem with KCSAN, where the compiler would
> tail-call-optimize __tsan_X instrumentation.
Those are compiler-generated calls, right? When those are generated the
compilation unit (and whatever it has included) might not have provided
a prototype anyway, and the compiler has special knowledge of the
functions, so it feels like the compiler would need to inhibit TCO here
for this to be robust. For their intended usage subjecting them to TCO
doesn't seem to make sense AFAICT.
I suspect that compilers have some way of handling that; otherwise I'd
expect to have heard stories of mcount/fentry calls getting TCO'd and
causing problems. So maybe there's an easy fix there?
> This would mean that KCSAN runtime functions ended up in the trace,
> but the function where the access happened would not. However, I don't
> care about the runtime functions, and instead want to see the function
> where the access happened. In that case, I'd like to just mark
> __tsan_X and any other kcsan instrumentation functions as
> do-not-tail-call-optimize, which would solve the problem.
I understand why we don't want to TCO these calls, but given the calls
are implicitly generated, I strongly suspect it's better to fix the
implicit call generation to not be TCO'd to begin with.
> The solution today is that when you compile a kernel with KCSAN, every
> instrumented TU is compiled with -fno-optimize-sibling-calls. The
> better solution would be to just mark KCSAN runtime functions somehow,
> but permit tail calling other things. Although, I probably still want
> to see the full trace, and would decide that having
> -fno-optimize-sibling-calls is a small price to pay in a
> debug-only-kernel to get complete traces.
>
> > ... if K were marked in this way, and J was compiled with visibility of
> > this, J would stick around, but J's callers might not, and so the a
> > trace might see:
> >
> > A->J->K
> >
> > ... do you just care about the final caller, i.e. you just need
> > certainty that J will be in the trace?
>
> Yes. But maybe it's a special problem that only sanitizers have.
I reckon for basically any instrumentation we don't want calls to be
TCO'd, though I'm not immediately sure of cases beyond sanitizers and
mcount/fentry.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Marco Elver @ 2021-03-04 19:01 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Will Deacon, LKML, broonie, Paul Mackerras,
kasan-dev, linux-toolchains, linuxppc-dev, Linux ARM
In-Reply-To: <20210304185148.GE60457@C02TD0UTHF1T.local>
On Thu, 4 Mar 2021 at 19:51, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Mar 04, 2021 at 07:22:53PM +0100, Marco Elver wrote:
> > On Thu, 4 Mar 2021 at 19:02, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Thu, Mar 04, 2021 at 06:25:33PM +0100, Marco Elver wrote:
> > > > On Thu, Mar 04, 2021 at 04:59PM +0000, Mark Rutland wrote:
> > > > > On Thu, Mar 04, 2021 at 04:30:34PM +0100, Marco Elver wrote:
> > > > > > On Thu, 4 Mar 2021 at 15:57, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > > > [adding Mark Brown]
> > > > > > >
> > > > > > > The bigger problem here is that skipping is dodgy to begin with, and
> > > > > > > this is still liable to break in some cases. One big concern is that
> > > > > > > (especially with LTO) we cannot guarantee the compiler will not inline
> > > > > > > or outline functions, causing the skipp value to be too large or too
> > > > > > > small. That's liable to happen to callers, and in theory (though
> > > > > > > unlikely in practice), portions of arch_stack_walk() or
> > > > > > > stack_trace_save() could get outlined too.
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Will LTO and friends respect 'noinline'?
> > > > >
> > > > > I hope so (and suspect we'd have more problems otherwise), but I don't
> > > > > know whether they actually so.
> > > > >
> > > > > I suspect even with 'noinline' the compiler is permitted to outline
> > > > > portions of a function if it wanted to (and IIUC it could still make
> > > > > specialized copies in the absence of 'noclone').
> > > > >
> > > > > > One thing I also noticed is that tail calls would also cause the stack
> > > > > > trace to appear somewhat incomplete (for some of my tests I've
> > > > > > disabled tail call optimizations).
> > > > >
> > > > > I assume you mean for a chain A->B->C where B tail-calls C, you get a
> > > > > trace A->C? ... or is A going missing too?
> > > >
> > > > Correct, it's just the A->C outcome.
> > >
> > > I'd assumed that those cases were benign, e.g. for livepatching what
> > > matters is what can be returned to, so B disappearing from the trace
> > > isn't a problem there.
> > >
> > > Is the concern debugability, or is there a functional issue you have in
> > > mind?
> >
> > For me, it's just been debuggability, and reliable test cases.
> >
> > > > > > Is there a way to also mark a function non-tail-callable?
> > > > >
> > > > > I think this can be bodged using __attribute__((optimize("$OPTIONS")))
> > > > > on a caller to inhibit TCO (though IIRC GCC doesn't reliably support
> > > > > function-local optimization options), but I don't expect there's any way
> > > > > to mark a callee as not being tail-callable.
> > > >
> > > > I don't think this is reliable. It'd be
> > > > __attribute__((optimize("-fno-optimize-sibling-calls"))), but doesn't
> > > > work if applied to the function we do not want to tail-call-optimize,
> > > > but would have to be applied to the function that does the tail-calling.
> > >
> > > Yup; that's what I meant then I said you could do that on the caller but
> > > not the callee.
> > >
> > > I don't follow why you'd want to put this on the callee, though, so I
> > > think I'm missing something. Considering a set of functions in different
> > > compilation units:
> > >
> > > A->B->C->D->E->F->G->H->I->J->K
> >
> > I was having this problem with KCSAN, where the compiler would
> > tail-call-optimize __tsan_X instrumentation.
>
> Those are compiler-generated calls, right? When those are generated the
> compilation unit (and whatever it has included) might not have provided
> a prototype anyway, and the compiler has special knowledge of the
> functions, so it feels like the compiler would need to inhibit TCO here
> for this to be robust. For their intended usage subjecting them to TCO
> doesn't seem to make sense AFAICT.
>
> I suspect that compilers have some way of handling that; otherwise I'd
> expect to have heard stories of mcount/fentry calls getting TCO'd and
> causing problems. So maybe there's an easy fix there?
I agree, the compiler builtins should be handled by the compiler
directly, perhaps that was a bad example. But we also have "explicit
instrumentation", e.g. everything that's in <linux/instrumented.h>.
> > This would mean that KCSAN runtime functions ended up in the trace,
> > but the function where the access happened would not. However, I don't
> > care about the runtime functions, and instead want to see the function
> > where the access happened. In that case, I'd like to just mark
> > __tsan_X and any other kcsan instrumentation functions as
> > do-not-tail-call-optimize, which would solve the problem.
>
> I understand why we don't want to TCO these calls, but given the calls
> are implicitly generated, I strongly suspect it's better to fix the
> implicit call generation to not be TCO'd to begin with.
>
> > The solution today is that when you compile a kernel with KCSAN, every
> > instrumented TU is compiled with -fno-optimize-sibling-calls. The
> > better solution would be to just mark KCSAN runtime functions somehow,
> > but permit tail calling other things. Although, I probably still want
> > to see the full trace, and would decide that having
> > -fno-optimize-sibling-calls is a small price to pay in a
> > debug-only-kernel to get complete traces.
> >
> > > ... if K were marked in this way, and J was compiled with visibility of
> > > this, J would stick around, but J's callers might not, and so the a
> > > trace might see:
> > >
> > > A->J->K
> > >
> > > ... do you just care about the final caller, i.e. you just need
> > > certainty that J will be in the trace?
> >
> > Yes. But maybe it's a special problem that only sanitizers have.
>
> I reckon for basically any instrumentation we don't want calls to be
> TCO'd, though I'm not immediately sure of cases beyond sanitizers and
> mcount/fentry.
Thinking about this more, I think it's all debugging tools. E.g.
lockdep, if you lock/unlock at the end of a function, you might tail
call into lockdep. If the compiler applies TCO, and lockdep determines
there's a bug and then shows a trace, you'll have no idea where the
actual bug is. The kernel has lots of debugging facilities that add
instrumentation in this way. So perhaps it's a general debugging-tool
problem (rather than just sanitizers).
Thanks,
-- Marco
^ 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 19:24 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Mark Rutland, Marco Elver, Catalin Marinas, linuxppc-dev, LKML,
kasan-dev, Mark Brown, Paul Mackerras, linux-toolchains,
Will Deacon, Linux ARM
In-Reply-To: <CAKwvOd=wBArMwvtDC8zV-QjQa5UuwWoxksQ8j+hUCZzbEAn+Fw@mail.gmail.com>
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")
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.
Segher
^ permalink raw reply
* Re: [PATCH v2 7/7] ASoC: dt-bindings: imx-rpmsg: Add binding doc for rpmsg machine driver
From: Rob Herring @ 2021-03-04 20:04 UTC (permalink / raw)
To: Shengjiu Wang
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: <CAA+D8ANOv91jr4381Acz1B2mZ6=Mx2J_2CMTGXmPKztv7bMjPA@mail.gmail.com>
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.
Rob
^ permalink raw reply
* 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
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