* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
From: Dave Martin @ 2020-05-19 13:21 UTC (permalink / raw)
To: Will Deacon
Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel,
Daniel Kiss
In-Reply-To: <20200519121818.14511-2-will@kernel.org>
On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote:
> For better or worse, GDB relies on the exact instruction sequence in the
> VDSO sigreturn trampoline in order to unwind from signals correctly.
Are you sure? I'm struggling to find the relevant code in gdb.
> Commit 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building
> the kernel with BTI") unfortunately added a BTI C instruction to the
> start of __kernel_rt_sigreturn, which breaks this check. Thankfully,
> it's also not required, since the trampoline is called from a RET
> instruction when returning from the signal handler
>
> Remove the unnecessary BTI C instruction from __kernel_rt_sigreturn.
>
> Cc: Dave Martin <dave.martin@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Daniel Kiss <daniel.kiss@arm.com>
> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
> Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> index 3fb13b81f780..83ac284dae79 100644
> --- a/arch/arm64/kernel/vdso/sigreturn.S
> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> @@ -15,7 +15,14 @@
> .text
>
> nop
> -SYM_FUNC_START(__kernel_rt_sigreturn)
> +/*
> + * GDB relies on being able to identify the sigreturn instruction sequence to
> + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
> + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully,
> + * this function is only ever called from a RET and so omitting the landing pad
> + * is perfectly fine.
> + */
> +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
> .cfi_startproc
> .cfi_signal_frame
> .cfi_def_cfa x29, 0
> --
> 2.26.2.761.g0e0b3e54be-goog
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Russell King - ARM Linux admin @ 2020-05-19 13:12 UTC (permalink / raw)
To: Lukasz Stelmach
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
Linux-Renesas, Chris Brandt, Rob Herring, Geert Uytterhoeven,
Uwe Kleine-König, Eric Miao, Dmitry Osipenko, Ard Biesheuvel,
Linux ARM, Marek Szyprowski
In-Reply-To: <dleftjzha43x8q.fsf%l.stelmach@samsung.com>
On Tue, May 19, 2020 at 02:49:57PM +0200, Lukasz Stelmach wrote:
> It was <2020-05-19 wto 13:27>, when Russell King - ARM Linux admin wrote:
> > On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
> >> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> >>> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> >>>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> >>>> <linux@armlinux.org.uk> wrote:
> >>>>> On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> >>>>>> On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
> >>>>>>> It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
> >>>>>>>> Currently, the start address of physical memory is obtained by masking
> >>>>>>>> the program counter with a fixed mask of 0xf8000000. This mask value
> >>>>>>>> was chosen as a balance between the requirements of different platforms.
> >>>>>>>> However, this does require that the start address of physical memory is
> >>>>>>>> a multiple of 128 MiB, precluding booting Linux on platforms where this
> >>>>>>>> requirement is not fulfilled.
> >>>>>>>>
> >>>>>>>> Fix this limitation by obtaining the start address from the DTB instead,
> >>>>>>>> if available (either explicitly passed, or appended to the kernel).
> >>>>>>>> Fall back to the traditional method when needed.
> [...]
> >>>>>>> Apparently reading physical memory layout from DTB breaks crashdump
> >>>>>>> kernels. A crashdump kernel is loaded into a region of memory, that is
> >>>>>>> reserved in the original (i.e. to be crashed) kernel. The reserved
> >>>>>>> region is large enough for the crashdump kernel to run completely inside
> >>>>>>> it and don't modify anything outside it, just read and dump the remains
> >>>>>>> of the crashed kernel. Using the information from DTB makes the
> >>>>>>> decompressor place the kernel outside of the dedicated region.
> >>>>>>>
> >>>>>>> The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
> >>>>>>> 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
> >>>>>>> it is decompressed to 0x00008000 (see r4 reported before jumping from
> >>>>>>> within __enter_kernel). If I were to suggest something, there need to be
> >>>>>>> one more bit of information passed in the DTB telling the decompressor
> >>>>>>> to use the old masking technique to determain kernel address. It would
> >>>>>>> be set in the DTB loaded along with the crashdump kernel.
> [...]
> >>>>>> Describing "to use the old masking technique" sounds a bit hackish to me.
> >>>>>> I guess it cannot just restrict the /memory node to the reserved region,
> >>>>>> as the crashkernel needs to be able to dump the remains of the crashed
> >>>>>> kernel, which lie outside this region.
> >>>>>
> >>>>> Correct.
> >>>>>
> >>>>>> However, something under /chosen should work.
> >>>>>
> >>>>> Yet another sticky plaster...
> >>>>
> >>>> IMHO the old masking technique is the hacky solution covered by
> >>>> plasters.
> >>>
> >>> One line of code is not "covered by plasters". There are no plasters.
> >>> It's a solution that works for 99.99% of people, unlike your approach
> >>> that has had a stream of issues over the last four months, and has
> >>> required many reworks of the code to fix each one. That in itself
> >>> speaks volumes about the suitability of the approach.
> >>
> >> As I have been working with kexec code (patches soon) I would like to
> >> defend the DT approach a bit. It allows to avoid zImage relocation when
> >> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
> >> small either and moving it around takes some time.
> >
> > ... which is something that has been supported for a very long time,
> > before the days of DT.
>
> How? If a decompressed kernel requires >128M and a bootloader would like
> to put a zImage high enough to *avoid* copying it once again, then the
> decompressor can't see any memory below the 128M window it starts in and
> can't decompress the kernel there.
Do you have such a large kernel? It would be rather inefficient as
branch instructions could not be used; every function call would have
to be indirect. The maximum is +/- 32MB for a branch.
> If we do not care about copying
> zImage, then, indeed, everything works fine as it is today. You are
> most probably right 99% doesn't require 128M kernel, but the case is
> IMHO obvious enough, that it should be adressed somehow.
If I have a kernel in excess of 4GB... "it should be addressed somehow"!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
From: Dave P Martin @ 2020-05-19 13:09 UTC (permalink / raw)
To: Will Deacon
Cc: Tamas Zsoldos, Mark Brown, kernel-team@android.com,
linux-arm-kernel@lists.infradead.org, Daniel Kiss
In-Reply-To: <20200519121818.14511-4-will@kernel.org>
On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote:
> Daniel reports that the .cfi_startproc is misplaced for the sigreturn
> trampoline, which causes LLVM's unwinder to misbehave:
>
> | I run into this with LLVM’s unwinder.
> | This combination was always broken.
>
> This prompted Dave to realise that our CFI directives are contradictory,
> as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter
> unconditionally identifying the interrupted context as opposed to the
> values in the sigcontext.
>
> Rework the CFI directives so that we only use .cfi_signal_frame, and
> include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
>
> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
> Reported-by: Dave Martin <dave.martin@arm.com>
> Reported-by: Daniel Kiss <daniel.kiss@arm.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> arch/arm64/kernel/vdso/sigreturn.S | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> index 7853fa9692f6..28b33f7d0604 100644
> --- a/arch/arm64/kernel/vdso/sigreturn.S
> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> @@ -14,6 +14,9 @@
>
> .text
>
> +/* Ensure that the mysterious NOP can be associated with a function. */
> + .cfi_startproc
> + .cfi_signal_frame
> /*
> * This mysterious NOP is required for some unwinders that subtract one from
> * the return address in order to identify the calling function.
> @@ -28,11 +31,6 @@
> * is perfectly fine.
> */
> SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
> - .cfi_startproc
> - .cfi_signal_frame
> - .cfi_def_cfa x29, 0
> - .cfi_offset x29, 0 * 8
> - .cfi_offset x30, 1 * 8
Having thought about this again, I think it might be better to stick to
the original version.
If the signal handler is halfway through mungeing the sigcontext then
backtracing using sigcontext won't be reliable.
In any case, if something in the interrupted code caused the signal, the
backtrace of the old stack is likely to me more useful, and that's what
x29 will give us. If there's no old stack because we blew it away,
that's too bad.
Plus, in the absence of any spec that says exactly what
.cfi_signal_frame means*, we probably don't want to rock the boat.
Cheers
---Dave
[*] assumption, but the arch ABI and Dwarf specs are unlikely to cover
this, and Linux doesn't go in for specs.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 2/4] kasan: record and print the free track
From: Dmitry Vyukov @ 2020-05-19 13:00 UTC (permalink / raw)
To: Walter Wu
Cc: wsd_upstream, linux-mediatek, LKML, kasan-dev, Linux-MM,
Alexander Potapenko, Andrey Ryabinin, Linux ARM
In-Reply-To: <20200519022517.24182-1-walter-zh.wu@mediatek.com>
On Tue, May 19, 2020 at 4:25 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> Move free track from slub alloc meta-data to slub free meta-data in
> order to make struct kasan_free_meta size is 16 bytes. It is a good
> size because it is the minimal redzone size and a good number of
> alignment.
>
> For free track in generic KASAN, we do the modification in struct
> kasan_alloc_meta and kasan_free_meta:
> - remove free track from kasan_alloc_meta.
> - add free track into kasan_free_meta.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
>
> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> ---
> mm/kasan/common.c | 22 ++--------------------
> mm/kasan/generic.c | 18 ++++++++++++++++++
> mm/kasan/kasan.h | 7 +++++++
> mm/kasan/report.c | 20 --------------------
> mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 64 insertions(+), 40 deletions(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 8bc618289bb1..47b53912f322 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -51,7 +51,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags)
> return stack_depot_save(entries, nr_entries, flags);
> }
>
> -static inline void set_track(struct kasan_track *track, gfp_t flags)
> +void kasan_set_track(struct kasan_track *track, gfp_t flags)
> {
> track->pid = current->pid;
> track->stack = kasan_save_stack(flags);
> @@ -299,24 +299,6 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
>
> -
> -static void kasan_set_free_info(struct kmem_cache *cache,
> - void *object, u8 tag)
> -{
> - struct kasan_alloc_meta *alloc_meta;
> - u8 idx = 0;
> -
> - alloc_meta = get_alloc_info(cache, object);
> -
> -#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> - idx = alloc_meta->free_track_idx;
> - alloc_meta->free_pointer_tag[idx] = tag;
> - alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
> -#endif
> -
> - set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> -}
> -
> void kasan_poison_slab(struct page *page)
> {
> unsigned long i;
> @@ -492,7 +474,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
> KASAN_KMALLOC_REDZONE);
>
> if (cache->flags & SLAB_KASAN)
> - set_track(&get_alloc_info(cache, object)->alloc_track, flags);
> + kasan_set_track(&get_alloc_info(cache, object)->alloc_track, flags);
>
> return set_tag(object, tag);
> }
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 3372bdcaf92a..763d8a13e0ac 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -344,3 +344,21 @@ void kasan_record_aux_stack(void *addr)
> alloc_info->aux_stack[1] = alloc_info->aux_stack[0];
> alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
> }
> +
> +void kasan_set_free_info(struct kmem_cache *cache,
> + void *object, u8 tag)
> +{
> + struct kasan_free_meta *free_meta;
> +
> + free_meta = get_free_info(cache, object);
> + kasan_set_track(&free_meta->free_track, GFP_NOWAIT);
> +}
> +
> +struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> + void *object, u8 tag)
> +{
> + struct kasan_free_meta *free_meta;
> +
> + free_meta = get_free_info(cache, object);
> + return &free_meta->free_track;
> +}
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index a7391bc83070..ad897ec36545 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -127,6 +127,9 @@ struct kasan_free_meta {
> * Otherwise it might be used for the allocator freelist.
> */
> struct qlist_node quarantine_link;
> +#ifdef CONFIG_KASAN_GENERIC
> + struct kasan_track free_track;
> +#endif
> };
>
> struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
> @@ -168,6 +171,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip);
> struct page *kasan_addr_to_page(const void *addr);
>
> depot_stack_handle_t kasan_save_stack(gfp_t flags);
> +void kasan_set_track(struct kasan_track *track, gfp_t flags);
> +void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
> +struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> + void *object, u8 tag);
>
> #if defined(CONFIG_KASAN_GENERIC) && \
> (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 6f8f2bf8f53b..96d2657fe70f 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -159,26 +159,6 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
> (void *)(object_addr + cache->object_size));
> }
>
> -static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> - void *object, u8 tag)
> -{
> - struct kasan_alloc_meta *alloc_meta;
> - int i = 0;
> -
> - alloc_meta = get_alloc_info(cache, object);
> -
> -#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> - for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> - if (alloc_meta->free_pointer_tag[i] == tag)
> - break;
> - }
> - if (i == KASAN_NR_FREE_STACKS)
> - i = alloc_meta->free_track_idx;
> -#endif
> -
> - return &alloc_meta->free_track[i];
> -}
> -
> #ifdef CONFIG_KASAN_GENERIC
> static void print_stack(depot_stack_handle_t stack)
> {
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index 25b7734e7013..201dee5d6ae0 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -162,3 +162,40 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
> kasan_poison_shadow((void *)addr, size, tag);
> }
> EXPORT_SYMBOL(__hwasan_tag_memory);
> +
> +void kasan_set_free_info(struct kmem_cache *cache,
> + void *object, u8 tag)
> +{
> + struct kasan_alloc_meta *alloc_meta;
> + u8 idx = 0;
> +
> + alloc_meta = get_alloc_info(cache, object);
> +
> +#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> + idx = alloc_meta->free_track_idx;
> + alloc_meta->free_pointer_tag[idx] = tag;
> + alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
> +#endif
> +
> + kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> +}
> +
> +struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> + void *object, u8 tag)
> +{
> + struct kasan_alloc_meta *alloc_meta;
> + int i = 0;
> +
> + alloc_meta = get_alloc_info(cache, object);
> +
> +#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> + for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> + if (alloc_meta->free_pointer_tag[i] == tag)
> + break;
> + }
> + if (i == KASAN_NR_FREE_STACKS)
> + i = alloc_meta->free_track_idx;
> +#endif
> +
> + return &alloc_meta->free_track[i];
> +}
Hi Walter,
FTR I've uploaded this for review purposes here:
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2458
Diff from the previous version is available as:
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2458/1..2
I've tested this locally and with syzkaller. This is 🔥🔥🔥:
[ 80.583021][ C3] Freed by task 0:
[ 80.583480][ C3] kasan_save_stack+0x1b/0x40 mm/kasan/common.c:49
[ 80.584056][ C3] kasan_set_track+0x1c/0x30 mm/kasan/common.c:57
[ 80.584617][ C3] kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:354
[ 80.585221][ C3] __kasan_slab_free+0xd8/0x120 mm/kasan/common.c:438
[ 80.585814][ C3] __cache_free mm/slab.c:3426 [inline]
[ 80.585814][ C3] kfree+0x10b/0x2b0 mm/slab.c:3757
[ 80.586291][ C3] kasan_rcu_reclaim+0x16/0x43 [test_kasan]
[ 80.587009][ C3] rcu_do_batch kernel/rcu/tree.c:2207 [inline]
[ 80.587009][ C3] rcu_core+0x59f/0x1370 kernel/rcu/tree.c:2434
[ 80.587537][ C3] __do_softirq+0x26c/0x9fa kernel/softirq.c:292
[ 80.588085][ C3]
[ 80.588367][ C3] Last one call_rcu() call stack:
[ 80.589052][ C3] kasan_save_stack+0x1b/0x40 mm/kasan/common.c:49
[ 80.589622][ C3] kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:345
[ 80.590254][ C3] __call_rcu kernel/rcu/tree.c:2672 [inline]
[ 80.590254][ C3] call_rcu+0x14f/0x7f0 kernel/rcu/tree.c:2746
[ 80.590782][ C3] kasan_rcu_uaf+0xe4/0xeb [test_kasan]
[ 80.591697][ C3] kmalloc_tests_init+0xbc/0x1097 [test_kasan]
[ 80.592900][ C3] do_one_initcall+0x10a/0x7d0 init/main.c:1196
[ 80.593494][ C3] do_init_module+0x1e6/0x6d0 kernel/module.c:3539
[ 80.594066][ C3] load_module+0x7464/0x9450 kernel/module.c:3890
[ 80.594626][ C3] __do_sys_init_module+0x1e3/0x220 kernel/module.c:3953
[ 80.595265][ C3] do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
[ 80.595822][ C3] entry_SYSCALL_64_after_hwframe+0x49/0xb3
Overall this looks very good to me.
But there is one aspect that bothers me. In the previous patch you had
code that returned NULL from kasan_get_free_track() if the object is
live (which means free meta is not available, it's occupied by object
data). Now you dropped that code, but I think we still need it.
Otherwise we cast user object data to free meta and print the free
stack/pid from whatever garbage is there. This may lead to very
confusing output and potentially to crashes in stackdepot.
What do you think about this patch on top of your patches?
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2478
This way we very precisely mark the period of time when the object has
free track live and set.
If it looks good to you, feel free to incorporate it into your series.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Lukasz Stelmach @ 2020-05-19 12:49 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
Linux-Renesas, Chris Brandt, Rob Herring, Geert Uytterhoeven,
Uwe Kleine-König, Eric Miao, Dmitry Osipenko, Ard Biesheuvel,
Linux ARM, Marek Szyprowski
In-Reply-To: <20200519122706.GC1551@shell.armlinux.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 4262 bytes --]
It was <2020-05-19 wto 13:27>, when Russell King - ARM Linux admin wrote:
> On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
>> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
>>> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
>>>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
>>>> <linux@armlinux.org.uk> wrote:
>>>>> On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
>>>>>> On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>>>>>>> It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
>>>>>>>> Currently, the start address of physical memory is obtained by masking
>>>>>>>> the program counter with a fixed mask of 0xf8000000. This mask value
>>>>>>>> was chosen as a balance between the requirements of different platforms.
>>>>>>>> However, this does require that the start address of physical memory is
>>>>>>>> a multiple of 128 MiB, precluding booting Linux on platforms where this
>>>>>>>> requirement is not fulfilled.
>>>>>>>>
>>>>>>>> Fix this limitation by obtaining the start address from the DTB instead,
>>>>>>>> if available (either explicitly passed, or appended to the kernel).
>>>>>>>> Fall back to the traditional method when needed.
[...]
>>>>>>> Apparently reading physical memory layout from DTB breaks crashdump
>>>>>>> kernels. A crashdump kernel is loaded into a region of memory, that is
>>>>>>> reserved in the original (i.e. to be crashed) kernel. The reserved
>>>>>>> region is large enough for the crashdump kernel to run completely inside
>>>>>>> it and don't modify anything outside it, just read and dump the remains
>>>>>>> of the crashed kernel. Using the information from DTB makes the
>>>>>>> decompressor place the kernel outside of the dedicated region.
>>>>>>>
>>>>>>> The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
>>>>>>> 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
>>>>>>> it is decompressed to 0x00008000 (see r4 reported before jumping from
>>>>>>> within __enter_kernel). If I were to suggest something, there need to be
>>>>>>> one more bit of information passed in the DTB telling the decompressor
>>>>>>> to use the old masking technique to determain kernel address. It would
>>>>>>> be set in the DTB loaded along with the crashdump kernel.
[...]
>>>>>> Describing "to use the old masking technique" sounds a bit hackish to me.
>>>>>> I guess it cannot just restrict the /memory node to the reserved region,
>>>>>> as the crashkernel needs to be able to dump the remains of the crashed
>>>>>> kernel, which lie outside this region.
>>>>>
>>>>> Correct.
>>>>>
>>>>>> However, something under /chosen should work.
>>>>>
>>>>> Yet another sticky plaster...
>>>>
>>>> IMHO the old masking technique is the hacky solution covered by
>>>> plasters.
>>>
>>> One line of code is not "covered by plasters". There are no plasters.
>>> It's a solution that works for 99.99% of people, unlike your approach
>>> that has had a stream of issues over the last four months, and has
>>> required many reworks of the code to fix each one. That in itself
>>> speaks volumes about the suitability of the approach.
>>
>> As I have been working with kexec code (patches soon) I would like to
>> defend the DT approach a bit. It allows to avoid zImage relocation when
>> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
>> small either and moving it around takes some time.
>
> ... which is something that has been supported for a very long time,
> before the days of DT.
How? If a decompressed kernel requires >128M and a bootloader would like
to put a zImage high enough to *avoid* copying it once again, then the
decompressor can't see any memory below the 128M window it starts in and
can't decompress the kernel there. If we do not care about copying
zImage, then, indeed, everything works fine as it is today. You are
most probably right 99% doesn't require 128M kernel, but the case is
IMHO obvious enough, that it should be adressed somehow.
Kind regards,
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
From: Mark Brown @ 2020-05-19 12:38 UTC (permalink / raw)
To: Will Deacon
Cc: Tamas Zsoldos, kernel-team, Dave Martin, linux-arm-kernel,
Daniel Kiss
In-Reply-To: <20200519121818.14511-2-will@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 1403 bytes --]
On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote:
> Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")
I'd say it's the annotation conversion not this, and also that the
bikeshed would be most fetching in orange.
c91db232da484851 (arm64: vdso: Convert to modern assembler annotations)
> -SYM_FUNC_START(__kernel_rt_sigreturn)
> +/*
> + * GDB relies on being able to identify the sigreturn instruction sequence to
> + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
> + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully,
> + * this function is only ever called from a RET and so omitting the landing pad
> + * is perfectly fine.
> + */
> +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
Shouldn't this be a SYM_CODE_START()? It's the same thing as above
currently and we'll break an awful lot more if we change what it does in
a way that affects the code, plus the use of CODE basically says the
above - it's a "this is non-standard and we know exactly what we're
doing, don't mess with it" annotation. If not then it'd be good to
cover that in the comment since otherwise this seems like it's asking
for a cleanup, we shouldn't really have raw SYM_START() in code.
I guess we also ought to annotate the 32 bit sigreturns as CODE too,
though it's academic there without BTI.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Russell King - ARM Linux admin @ 2020-05-19 12:27 UTC (permalink / raw)
To: Lukasz Stelmach
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
Linux-Renesas, Chris Brandt, Rob Herring, Geert Uytterhoeven,
Uwe Kleine-König, Eric Miao, Dmitry Osipenko, Ard Biesheuvel,
Linux ARM, Marek Szyprowski
In-Reply-To: <dleftj5zcs5d6e.fsf%l.stelmach@samsung.com>
On Tue, May 19, 2020 at 02:20:25PM +0200, Lukasz Stelmach wrote:
> It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> > On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> >> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> >> <linux@armlinux.org.uk> wrote:
> >> > On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> >> > > On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
> >> > > > It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
> >> > > > > Currently, the start address of physical memory is obtained by masking
> >> > > > > the program counter with a fixed mask of 0xf8000000. This mask value
> >> > > > > was chosen as a balance between the requirements of different platforms.
> >> > > > > However, this does require that the start address of physical memory is
> >> > > > > a multiple of 128 MiB, precluding booting Linux on platforms where this
> >> > > > > requirement is not fulfilled.
> >> > > > >
> >> > > > > Fix this limitation by obtaining the start address from the DTB instead,
> >> > > > > if available (either explicitly passed, or appended to the kernel).
> >> > > > > Fall back to the traditional method when needed.
> >> > > > >
> >> > > > > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> >> > > > > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> >> > > > > i.e. not at a multiple of 128 MiB.
> >> > > > >
> >> > > > > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> >> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> >> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> >> > > > > ---
> >> > > >
> >> > > > [...]
> >> > > >
> >> > > > Apparently reading physical memory layout from DTB breaks crashdump
> >> > > > kernels. A crashdump kernel is loaded into a region of memory, that is
> >> > > > reserved in the original (i.e. to be crashed) kernel. The reserved
> >> > > > region is large enough for the crashdump kernel to run completely inside
> >> > > > it and don't modify anything outside it, just read and dump the remains
> >> > > > of the crashed kernel. Using the information from DTB makes the
> >> > > > decompressor place the kernel outside of the dedicated region.
> >> > > >
> >> > > > The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
> >> > > > 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
> >> > > > it is decompressed to 0x00008000 (see r4 reported before jumping from
> >> > > > within __enter_kernel). If I were to suggest something, there need to be
> >> > > > one more bit of information passed in the DTB telling the decompressor
> >> > > > to use the old masking technique to determain kernel address. It would
> >> > > > be set in the DTB loaded along with the crashdump kernel.
> >> > >
> >> > > Shouldn't the DTB passed to the crashkernel describe which region of
> >> > > memory is to be used instead?
> >> >
> >> > Definitely not. The crashkernel needs to know where the RAM in the
> >> > machine is, so that it can create a coredump of the crashed kernel.
> >>
> >> So the DTB should describe both ;-)
> >>
> >> > > Describing "to use the old masking technique" sounds a bit hackish to me.
> >> > > I guess it cannot just restrict the /memory node to the reserved region,
> >> > > as the crashkernel needs to be able to dump the remains of the crashed
> >> > > kernel, which lie outside this region.
> >> >
> >> > Correct.
> >> >
> >> > > However, something under /chosen should work.
> >> >
> >> > Yet another sticky plaster...
> >>
> >> IMHO the old masking technique is the hacky solution covered by
> >> plasters.
> >
> > One line of code is not "covered by plasters". There are no plasters.
> > It's a solution that works for 99.99% of people, unlike your approach
> > that has had a stream of issues over the last four months, and has
> > required many reworks of the code to fix each one. That in itself
> > speaks volumes about the suitability of the approach.
>
> As I have been working with kexec code (patches soon) I would like to
> defend the DT approach a bit. It allows to avoid zImage relocation when
> a decompressed kernel is larger than ~128MiB. In such case zImage isn't
> small either and moving it around takes some time.
... which is something that has been supported for a very long time,
before the days of DT.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: at91: sama5d2_xplained: Add aliases for the dedicated I2C IPs
From: Alexandre Belloni @ 2020-05-19 12:27 UTC (permalink / raw)
To: Tudor.Ambarus
Cc: devicetree, linux-kernel, Ludovic.Desroches, robh+dt,
linux-arm-kernel
In-Reply-To: <20200518114802.253660-1-tudor.ambarus@microchip.com>
On 18/05/2020 11:49:21+0000, Tudor.Ambarus@microchip.com wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> The sama5d2 SoC has two dedicated I2C IPs that are enabled on
> sama5d2_xplained. Add alias for the i2c devices to not rely on
> probe order for the i2c device numbering.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> arch/arm/boot/dts/at91-sama5d2_xplained.dts | 2 ++
> 1 file changed, 2 insertions(+)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Lukasz Stelmach @ 2020-05-19 12:20 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Linux Kernel Mailing List,
Linux-Renesas, Chris Brandt, Rob Herring, Geert Uytterhoeven,
Uwe Kleine-König, Eric Miao, Dmitry Osipenko, Ard Biesheuvel,
Linux ARM, Marek Szyprowski
In-Reply-To: <20200519114329.GB1551@shell.armlinux.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 4479 bytes --]
It was <2020-05-19 wto 12:43>, when Russell King - ARM Linux admin wrote:
> On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
>> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
>> <linux@armlinux.org.uk> wrote:
>> > On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
>> > > On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>> > > > It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
>> > > > > Currently, the start address of physical memory is obtained by masking
>> > > > > the program counter with a fixed mask of 0xf8000000. This mask value
>> > > > > was chosen as a balance between the requirements of different platforms.
>> > > > > However, this does require that the start address of physical memory is
>> > > > > a multiple of 128 MiB, precluding booting Linux on platforms where this
>> > > > > requirement is not fulfilled.
>> > > > >
>> > > > > Fix this limitation by obtaining the start address from the DTB instead,
>> > > > > if available (either explicitly passed, or appended to the kernel).
>> > > > > Fall back to the traditional method when needed.
>> > > > >
>> > > > > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
>> > > > > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
>> > > > > i.e. not at a multiple of 128 MiB.
>> > > > >
>> > > > > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
>> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
>> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
>> > > > > ---
>> > > >
>> > > > [...]
>> > > >
>> > > > Apparently reading physical memory layout from DTB breaks crashdump
>> > > > kernels. A crashdump kernel is loaded into a region of memory, that is
>> > > > reserved in the original (i.e. to be crashed) kernel. The reserved
>> > > > region is large enough for the crashdump kernel to run completely inside
>> > > > it and don't modify anything outside it, just read and dump the remains
>> > > > of the crashed kernel. Using the information from DTB makes the
>> > > > decompressor place the kernel outside of the dedicated region.
>> > > >
>> > > > The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
>> > > > 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
>> > > > it is decompressed to 0x00008000 (see r4 reported before jumping from
>> > > > within __enter_kernel). If I were to suggest something, there need to be
>> > > > one more bit of information passed in the DTB telling the decompressor
>> > > > to use the old masking technique to determain kernel address. It would
>> > > > be set in the DTB loaded along with the crashdump kernel.
>> > >
>> > > Shouldn't the DTB passed to the crashkernel describe which region of
>> > > memory is to be used instead?
>> >
>> > Definitely not. The crashkernel needs to know where the RAM in the
>> > machine is, so that it can create a coredump of the crashed kernel.
>>
>> So the DTB should describe both ;-)
>>
>> > > Describing "to use the old masking technique" sounds a bit hackish to me.
>> > > I guess it cannot just restrict the /memory node to the reserved region,
>> > > as the crashkernel needs to be able to dump the remains of the crashed
>> > > kernel, which lie outside this region.
>> >
>> > Correct.
>> >
>> > > However, something under /chosen should work.
>> >
>> > Yet another sticky plaster...
>>
>> IMHO the old masking technique is the hacky solution covered by
>> plasters.
>
> One line of code is not "covered by plasters". There are no plasters.
> It's a solution that works for 99.99% of people, unlike your approach
> that has had a stream of issues over the last four months, and has
> required many reworks of the code to fix each one. That in itself
> speaks volumes about the suitability of the approach.
As I have been working with kexec code (patches soon) I would like to
defend the DT approach a bit. It allows to avoid zImage relocation when
a decompressed kernel is larger than ~128MiB. In such case zImage isn't
small either and moving it around takes some time.
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iommu/mediatek-v1: Fix a build warning for a unused variable 'data'
From: Joerg Roedel @ 2020-05-19 12:19 UTC (permalink / raw)
To: Yong Wu
Cc: youlin.pei, anan.sun, srv_heupstream, Will Deacon, linux-kernel,
iommu, linux-mediatek, Matthias Brugger, Robin Murphy,
linux-arm-kernel
In-Reply-To: <1589875064-662-1-git-send-email-yong.wu@mediatek.com>
On Tue, May 19, 2020 at 03:57:44PM +0800, Yong Wu wrote:
> This patch fixes a build warning:
> drivers/iommu/mtk_iommu_v1.c: In function 'mtk_iommu_release_device':
> >> drivers/iommu/mtk_iommu_v1.c:467:25: warning: variable 'data' set but
> >> not used [-Wunused-but-set-variable]
> 467 | struct mtk_iommu_data *data;
> | ^~~~
>
> It's reported at:
> https://lore.kernel.org/linux-iommu/202005191458.gY38V8bU%25lkp@intel.com/T/#u
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> drivers/iommu/mtk_iommu_v1.c | 2 --
> 1 file changed, 2 deletions(-)
Applied, thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
In-Reply-To: <20200519121818.14511-1-will@kernel.org>
Daniel reports that the .cfi_startproc is misplaced for the sigreturn
trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.
This prompted Dave to realise that our CFI directives are contradictory,
as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter
unconditionally identifying the interrupted context as opposed to the
values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and
include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Reported-by: Dave Martin <dave.martin@arm.com>
Reported-by: Daniel Kiss <daniel.kiss@arm.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kernel/vdso/sigreturn.S | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 7853fa9692f6..28b33f7d0604 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -14,6 +14,9 @@
.text
+/* Ensure that the mysterious NOP can be associated with a function. */
+ .cfi_startproc
+ .cfi_signal_frame
/*
* This mysterious NOP is required for some unwinders that subtract one from
* the return address in order to identify the calling function.
@@ -28,11 +31,6 @@
* is perfectly fine.
*/
SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
- .cfi_startproc
- .cfi_signal_frame
- .cfi_def_cfa x29, 0
- .cfi_offset x29, 0 * 8
- .cfi_offset x30, 1 * 8
mov x8, #__NR_rt_sigreturn
svc #0
.cfi_endproc
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn
From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
In-Reply-To: <20200519121818.14511-1-will@kernel.org>
Every so often we have to remind ourselves about the purpose of the
weird NOP instruction immediately preceding the sigreturn trampoline.
Add a short comment to state that it exists for some unwinders that
determine the caller address by subtracting from the return address.
Cc: Dave Martin <dave.martin@arm.com>
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kernel/vdso/sigreturn.S | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 83ac284dae79..7853fa9692f6 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -14,7 +14,12 @@
.text
- nop
+/*
+ * This mysterious NOP is required for some unwinders that subtract one from
+ * the return address in order to identify the calling function.
+ * Hack borrowed from arch/powerpc/kernel/vdso64/sigtramp.S.
+ */
+ nop // Mysterious NOP
/*
* GDB relies on being able to identify the sigreturn instruction sequence to
* unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
In-Reply-To: <20200519121818.14511-1-will@kernel.org>
For better or worse, GDB relies on the exact instruction sequence in the
VDSO sigreturn trampoline in order to unwind from signals correctly.
Commit 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building
the kernel with BTI") unfortunately added a BTI C instruction to the
start of __kernel_rt_sigreturn, which breaks this check. Thankfully,
it's also not required, since the trampoline is called from a RET
instruction when returning from the signal handler
Remove the unnecessary BTI C instruction from __kernel_rt_sigreturn.
Cc: Dave Martin <dave.martin@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 3fb13b81f780..83ac284dae79 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -15,7 +15,14 @@
.text
nop
-SYM_FUNC_START(__kernel_rt_sigreturn)
+/*
+ * GDB relies on being able to identify the sigreturn instruction sequence to
+ * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
+ * here, as it will emit a BTI C instruction and break the unwinder. Thankfully,
+ * this function is only ever called from a RET and so omitting the landing pad
+ * is perfectly fine.
+ */
+SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
.cfi_startproc
.cfi_signal_frame
.cfi_def_cfa x29, 0
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 0/3] arm64 sigreturn unwinding fixes
From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
Hi folks,
Here are a handful of sigreturn unwinding fixes, based on top of for-next/bti.
Note that I haven't confirmed the GDB breakage, I only spotted it by reading
the code.
Daniel, Tamas: please can you confirm that these fix your unwinding issues with
LLVM?
Given that this has always been broken and there's a risk of introducing
a new regression, I plan to queue these for 5.8 so that we can revert
bits if necessary.
Thanks,
Will
Cc: Dave Martin <dave.martin@arm.com>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: <kernel-team@android.com>
--->8
Will Deacon (3):
arm64: vdso: Don't prefix sigreturn trampoline with a BTI C
instruction
arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn
arm64: vdso: Fix CFI directives in sigreturn trampoline
arch/arm64/kernel/vdso/sigreturn.S | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Geert Uytterhoeven @ 2020-05-19 12:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Lukasz Stelmach,
Russell King - ARM Linux admin, Linux Kernel Mailing List,
Linux-Renesas, Chris Brandt, Rob Herring, Uwe Kleine-König,
Eric Miao, Dmitry Osipenko, Ard Biesheuvel, Linux ARM,
Marek Szyprowski
In-Reply-To: <CAK8P3a3H=7qx+Rz9sScTVCSMKWGwQ_ROnyoyK73A5yUd+_jbTw@mail.gmail.com>
Hi Arnd,
On Tue, May 19, 2020 at 1:28 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, May 19, 2020 at 1:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
>
> > >
> > > > However, something under /chosen should work.
> > >
> > > Yet another sticky plaster...
> >
> > IMHO the old masking technique is the hacky solution covered by
> > plasters.
> >
> > DT describes the hardware. In general, where to put the kernel is a
> > software policy, and thus doesn't belong in DT, except perhaps under
> > /chosen. But that would open another can of worms, as people usually
> > have no business in specifying where the kernel should be located.
> > In the crashkernel case, there is a clear separation between memory to
> > be used by the crashkernel, and memory to be solely inspected by the
> > crashkernel.
> >
> > Devicetree Specification, Release v0.3, Section 3.4 "/memory node" says:
> >
> > "The client program may access memory not covered by any memory
> > reservations (see section 5.3)"
> >
> > (Section 5.3 "Memory Reservation Block" only talks about structures in
> > the FDT, not about DTS)
> >
> > Hence according to the above, the crashkernel is rightfully allowed to
> > do whatever it wants with all memory under the /memory node.
> > However, there is also
> > Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
> > This suggests the crashkernel should be passed a DTB that contains a
> > /reserved-memory node, describing which memory cannot be used freely.
> > Then the decompressor needs to take this into account when deciding
> > where the put the kernel.
> >
> > Yes, the above requires changing code. But at least it provides a
> > path forward, getting rid of the fragile old masking technique.
>
> There is an existing "linux,usable-memory-range" property documented
> in Documentation/devicetree/bindings/chosen.txt, which as I understand
> is exactly what you are looking for, except that it is currently only
> documented for arm64.
Thank you, that looks appropriate!
It seems this is not really used by the early startup code.
Is that because the early startup code always runs in-place, and the
kernel image is not even copied?
> Would extending this to arm work?
Let's see.... Th arm early boot code seems to be more complex than the
arm64 code ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp
From: Lars Povlsen @ 2020-05-19 12:10 UTC (permalink / raw)
To: Mark Brown
Cc: devicetree, Alexandre Belloni, Andy Shevchenko, linux-kernel,
Serge Semin, linux-spi, Serge Semin, Wan Ahmad Zainie, SoC Team,
Rob Herring, Lars Povlsen, Microchip Linux Driver Support,
linux-arm-kernel
In-Reply-To: <20200519115829.GI4611@sirena.org.uk>
On 19/05/20 12:58, Mark Brown wrote:
> Date: Tue, 19 May 2020 12:58:29 +0100
> From: Mark Brown <broonie@kernel.org>
> To: Lars Povlsen <lars.povlsen@microchip.com>
> Cc: SoC Team <soc@kernel.org>, Rob Herring <robh+dt@kernel.org>, Microchip
> Linux Driver Support <UNGLinuxDriver@microchip.com>,
> linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
> linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
> Alexandre Belloni <alexandre.belloni@bootlin.com>, Serge Semin
> <Sergey.Semin@baikalelectronics.ru>, Serge Semin
> <fancer.lancer@gmail.com>, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Subject: Re: [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> On Tue, May 19, 2020 at 01:47:39PM +0200, Lars Povlsen wrote:
> > On 13/05/20 15:52, Mark Brown wrote:
>
> > > On Wed, May 13, 2020 at 04:00:25PM +0200, Lars Povlsen wrote:
> > > > This add DT bindings for the Microsemi/Microchip SPI controller used
> > > > in various SoC's. It describes the "mscc,ocelot-spi" and
> > > > "mscc,jaguar2-spi" bindings.
>
> > > That's not what this change does. It is removing the existing binding
> > > for Ocelot and Jaguar2 from the free format binding documentation and
>
> > The reason for doing this was due to the fact that I felt adding
> > Sparx5 support only cluttered the original driver even more.
>
> That's not the issue I'm pointing out there. The issue is that your
> changelog claims that the change does one thing and the change itself
> does something substantially different.
Ok, got it. I'll reword the changelog to be more precise.
Thanks again,
---Lars
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: Fix PTRACE_SYSEMU semantics
From: Catalin Marinas @ 2020-05-19 12:07 UTC (permalink / raw)
To: Will Deacon
Cc: will.deacon, linux-kernel, oleg, Keno Fischer, sudeep.holla,
linux-arm-kernel
In-Reply-To: <20200518114119.GB32394@willie-the-truck>
On Mon, May 18, 2020 at 12:41:20PM +0100, Will Deacon wrote:
> On Fri, May 15, 2020 at 06:22:53PM -0400, Keno Fischer wrote:
> > Quoth the man page:
> > ```
> > If the tracee was restarted by PTRACE_SYSCALL or PTRACE_SYSEMU, the
> > tracee enters syscall-enter-stop just prior to entering any system
> > call (which will not be executed if the restart was using
> > PTRACE_SYSEMU, regardless of any change made to registers at this
> > point or how the tracee is restarted after this stop).
> > ```
> >
> > The parenthetical comment is currently true on x86 and powerpc,
> > but not currently true on arm64. arm64 re-checks the _TIF_SYSCALL_EMU
> > flag after the syscall entry ptrace stop. However, at this point,
> > it reflects which method was used to re-start the syscall
> > at the entry stop, rather than the method that was used to reach it.
> > Fix that by recording the original flag before performing the ptrace
> > stop, bringing the behavior in line with documentation and x86/powerpc.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > ---
> > arch/arm64/kernel/ptrace.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index b3d3005d9515..b67b4d14aa17 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -1829,10 +1829,12 @@ static void tracehook_report_syscall(struct pt_regs *regs,
> >
> > int syscall_trace_enter(struct pt_regs *regs)
> > {
> > - if (test_thread_flag(TIF_SYSCALL_TRACE) ||
> > - test_thread_flag(TIF_SYSCALL_EMU)) {
> > + u32 flags = READ_ONCE(current_thread_info()->flags) &
> > + (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
> > +
> > + if (flags) {
>
> nit: but I'd rather the '&' operation was in the conditional so that the
> 'flags' variable holds all of the flags.
>
> With that:
>
> Acked-by: Will Deacon <will@kernel.org>
>
> Also needs:
>
> Cc: <stable@vger.kernel.org>
> Fixes: f086f67485c5 ("arm64: ptrace: add support for syscall emulation")
>
> Catalin -- can you pick this up for 5.7 please, with my 'nit' addressed?
I'll queue it with the above addressed. I think flags also needs to be
unsigned long rather than u32.
However, before sending the pull request, I'd like Sudeep to confirm
that it doesn't break his original use-case for this feature.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH -next] iommu/sun50i: Fix return value check in sun50i_iommu_probe()
From: Joerg Roedel @ 2020-05-19 12:06 UTC (permalink / raw)
To: Wei Yongjun
Cc: kernel-janitors, iommu, Maxime Ripard, linux-kernel, Hulk Robot,
Chen-Yu Tsai, Maxime Ripard, Philipp Zabel, linux-arm-kernel
In-Reply-To: <20200519091857.134170-1-weiyongjun1@huawei.com>
On Tue, May 19, 2020 at 09:18:57AM +0000, Wei Yongjun wrote:
> In case of error, the function devm_platform_ioremap_resource() returns
> ERR_PTR() not NULL. The NULL test in the return value check must be
> replaced with IS_ERR().
>
> Fixes: 4100b8c229b3 ("iommu: Add Allwinner H6 IOMMU driver")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/iommu/sun50i-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into spi-dw-mchp
From: Lars Povlsen @ 2020-05-19 12:05 UTC (permalink / raw)
To: Mark Brown
Cc: devicetree, Alexandre Belloni, linux-kernel, linux-spi, SoC Team,
Lars Povlsen, Microchip Linux Driver Support, linux-arm-kernel
In-Reply-To: <20200513151811.GL4803@sirena.org.uk>
On 13/05/20 16:18, Mark Brown wrote:
> Date: Wed, 13 May 2020 16:18:11 +0100
> From: Mark Brown <broonie@kernel.org>
> To: Lars Povlsen <lars.povlsen@microchip.com>
> Cc: SoC Team <soc@kernel.org>, Microchip Linux Driver Support
> <UNGLinuxDriver@microchip.com>, linux-spi@vger.kernel.org,
> devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-arm-kernel@lists.infradead.org, Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into
> spi-dw-mchp
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> On Wed, May 13, 2020 at 04:00:26PM +0200, Lars Povlsen wrote:
>
> > +config SPI_DW_MCHP
> > + tristate "Memory-mapped io interface driver using DW SPI core of MSCC SoCs"
> > + default y if ARCH_SPARX5
> > + default y if SOC_VCOREIII
>
> Why the default ys?
The SoC will typically boot from SPI... But its not a requirement per
se. I will remove it.
>
> > +++ b/drivers/spi/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o
> > obj-$(CONFIG_SPI_DLN2) += spi-dln2.o
> > obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o
> > obj-$(CONFIG_SPI_DW_MMIO) += spi-dw-mmio.o
> > +obj-$(CONFIG_SPI_DW_MCHP) += spi-dw-mchp.o
> > obj-$(CONFIG_SPI_DW_PCI) += spi-dw-midpci.o
> > spi-dw-midpci-objs := spi-dw-pci.o spi-dw-mid.o
> > obj-$(CONFIG_SPI_EFM32) += spi-efm32.o
>
> Please keep the file alphabetically sorted.
>
Noted.
> > +++ b/drivers/spi/spi-dw-mchp.c
> > @@ -0,0 +1,232 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Memory-mapped interface driver for MSCC SoCs
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.
Sure, I can do that. The presented form matches that of the other
spi-dw-* drivers, but I can see other using // blocks. Ack.
>
> > +#define MAX_CS 4
>
> This should be namespaced.
Ack.
>
> > + rx_sample_dly = 0;
> > + device_property_read_u32(&pdev->dev, "spi-rx-delay-us", &rx_sample_dly);
> > + dws->rx_sample_dly = DIV_ROUND_UP(rx_sample_dly,
> > + (dws->max_freq / 1000000));
>
> If this is a standard feature of the DesignWare IP why parse it here and
> not in the generic code?
This is a standard feature of the DesignWare IP, so good suggestion. I
will arrange with Serge.
Thank you for your comments!
---Lars
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Increase stack size to 16KB
From: Linus Walleij @ 2020-05-19 12:02 UTC (permalink / raw)
To: nrg02771; +Cc: Linux ARM
In-Reply-To: <828135836.20701492468756240.nrg02771@nifty.com>
On Tue, Apr 18, 2017 at 12:40 AM <nrg02771@nifty.com> wrote:
> I want to increase stack size to 16KB on freescale 32bit arm system.
(...)
> I modified the following kernel source code to increase stack size.
> I confirmed kernel work fine, but after mounted rootfs, init process
> dose not work with the following error message.
> I confirmed init process created without error, but it seems when
> executing init process the error occurs.
> Is it possible to increase stack size to 16KB with 32bit arm freescale
> environment?
>
> I confirmed OMAP arm 32bit environment work fine with the same modification.
I think I have solved this as part of my work on the KASan
support for ARMv4 and ARMv5.
Could you check this patch:
https://lore.kernel.org/linux-arm-kernel/20200515124808.213538-1-linus.walleij@linaro.org/
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp
From: Mark Brown @ 2020-05-19 11:58 UTC (permalink / raw)
To: Lars Povlsen
Cc: devicetree, Alexandre Belloni, Andy Shevchenko, linux-kernel,
Serge Semin, linux-spi, Serge Semin, Wan Ahmad Zainie, SoC Team,
Rob Herring, Microchip Linux Driver Support, linux-arm-kernel
In-Reply-To: <20200519114739.GD24801@soft-dev15.microsemi.net>
[-- Attachment #1.1: Type: text/plain, Size: 802 bytes --]
On Tue, May 19, 2020 at 01:47:39PM +0200, Lars Povlsen wrote:
> On 13/05/20 15:52, Mark Brown wrote:
> > On Wed, May 13, 2020 at 04:00:25PM +0200, Lars Povlsen wrote:
> > > This add DT bindings for the Microsemi/Microchip SPI controller used
> > > in various SoC's. It describes the "mscc,ocelot-spi" and
> > > "mscc,jaguar2-spi" bindings.
> > That's not what this change does. It is removing the existing binding
> > for Ocelot and Jaguar2 from the free format binding documentation and
> The reason for doing this was due to the fact that I felt adding
> Sparx5 support only cluttered the original driver even more.
That's not the issue I'm pointing out there. The issue is that your
changelog claims that the change does one thing and the change itself
does something substantially different.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp
From: Lars Povlsen @ 2020-05-19 11:47 UTC (permalink / raw)
To: Mark Brown
Cc: devicetree, Alexandre Belloni, Andy Shevchenko, linux-kernel,
Serge Semin, linux-spi, Serge Semin, Wan Ahmad Zainie, SoC Team,
Rob Herring, Lars Povlsen, Microchip Linux Driver Support,
linux-arm-kernel
In-Reply-To: <20200513145213.GJ4803@sirena.org.uk>
On 13/05/20 15:52, Mark Brown wrote:
> Date: Wed, 13 May 2020 15:52:13 +0100
> From: Mark Brown <broonie@kernel.org>
> To: Lars Povlsen <lars.povlsen@microchip.com>
> Cc: SoC Team <soc@kernel.org>, Rob Herring <robh+dt@kernel.org>, Microchip
> Linux Driver Support <UNGLinuxDriver@microchip.com>,
> linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
> linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
> Alexandre Belloni <alexandre.belloni@bootlin.com>, Serge Semin
> <Sergey.Semin@baikalelectronics.ru>, Serge Semin
> <fancer.lancer@gmail.com>, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Subject: Re: [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> On Wed, May 13, 2020 at 04:00:25PM +0200, Lars Povlsen wrote:
> > This add DT bindings for the Microsemi/Microchip SPI controller used
> > in various SoC's. It describes the "mscc,ocelot-spi" and
> > "mscc,jaguar2-spi" bindings.
>
> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > ---
> > .../bindings/spi/mscc,ocelot-spi.yaml | 60 +++++++++++++++++++
> > .../bindings/spi/snps,dw-apb-ssi.txt | 7 +--
>
> That's not what this change does. It is removing the existing binding
> for Ocelot and Jaguar2 from the free format binding documentation and
> adds some entirely separate YAML bindings for them. This conflicts with
> competing YAML conversions that both Serge Semin and Wan Ahmad Zainie
> (CCed) have in flight at the minute. It also doesn't strike me as a
> good idea to fork the bindings, what's the motivation for doing that?
The reason for doing this was due to the fact that I felt adding
Sparx5 support only cluttered the original driver even more.
And since I was adding a new driver, I needed to add bindings for it -
and (re)move the old ones.
I have become aware of Serge and Wan Ahmad's work, and will work out
something on top of that.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Lukasz Stelmach @ 2020-05-19 11:46 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Russell King - ARM Linux admin,
Linux Kernel Mailing List, Linux-Renesas, Chris Brandt,
Rob Herring, Uwe Kleine-König, Eric Miao, Dmitry Osipenko,
Ard Biesheuvel, Linux ARM, Marek Szyprowski
In-Reply-To: <CAMuHMdU5DG06G4H=+PH+OONMT_9oE==KS=wP+bLgY9xVCez6Ww@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3210 bytes --]
It was <2020-05-19 wto 13:21>, when Geert Uytterhoeven wrote:
> Hi Russell,
>
> CC devicetree
>
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
>> On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
>>> On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>>>> It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
>>>>> Currently, the start address of physical memory is obtained by masking
>>>>> the program counter with a fixed mask of 0xf8000000. This mask value
>>>>> was chosen as a balance between the requirements of different platforms.
>>>>> However, this does require that the start address of physical memory is
>>>>> a multiple of 128 MiB, precluding booting Linux on platforms where this
>>>>> requirement is not fulfilled.
>>>>>
>>>>> Fix this limitation by obtaining the start address from the DTB instead,
>>>>> if available (either explicitly passed, or appended to the kernel).
>>>>> Fall back to the traditional method when needed.
>>>>>
>>>>> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
>>>>> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
>>>>> i.e. not at a multiple of 128 MiB.
>>>>>
>>>>> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>> Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
>>>>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>> Apparently reading physical memory layout from DTB breaks crashdump
>>>> kernels. A crashdump kernel is loaded into a region of memory, that
>>>> is reserved in the original (i.e. to be crashed) kernel. The
>>>> reserved region is large enough for the crashdump kernel to run
>>>> completely inside it and don't modify anything outside it, just
>>>> read and dump the remains of the crashed kernel. Using the
>>>> information from DTB makes the decompressor place the kernel
>>>> outside of the dedicated region.
>>>>
>>>> The log below shows that a zImage and DTB are loaded at 0x18eb8000
>>>> and 0x193f6000 (physical). The kernel is expected to run at
>>>> 0x18008000, but it is decompressed to 0x00008000 (see r4 reported
>>>> before jumping from within __enter_kernel). If I were to suggest
>>>> something, there need to be one more bit of information passed in
>>>> the DTB telling the decompressor to use the old masking technique
>>>> to determain kernel address. It would be set in the DTB loaded
>>>> along with the crashdump kernel.
>>>
>>> Shouldn't the DTB passed to the crashkernel describe which region of
>>> memory is to be used instead?
>>
>> Definitely not. The crashkernel needs to know where the RAM in the
>> machine is, so that it can create a coredump of the crashed kernel.
>
> So the DTB should describe both ;-)
So we can do without the mem= cmdline option which is required
now. Sounds reasonable to me.
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] ARM: boot: Obtain start of physical memory from DTB
From: Russell King - ARM Linux admin @ 2020-05-19 11:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Grant Likely, Arnd Bergmann, Nicolas Pitre, Masahiro Yamada,
Bartlomiej Zolnierkiewicz, Lukasz Stelmach,
Linux Kernel Mailing List, Linux-Renesas, Chris Brandt,
Rob Herring, Uwe Kleine-König, Eric Miao, Dmitry Osipenko,
Ard Biesheuvel, Linux ARM, Marek Szyprowski
In-Reply-To: <CAMuHMdU5DG06G4H=+PH+OONMT_9oE==KS=wP+bLgY9xVCez6Ww@mail.gmail.com>
On Tue, May 19, 2020 at 01:21:09PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
>
> CC devicetree
>
> On Tue, May 19, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Tue, May 19, 2020 at 11:44:17AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 19, 2020 at 10:54 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
> > > > It was <2020-04-29 śro 10:21>, when Geert Uytterhoeven wrote:
> > > > > Currently, the start address of physical memory is obtained by masking
> > > > > the program counter with a fixed mask of 0xf8000000. This mask value
> > > > > was chosen as a balance between the requirements of different platforms.
> > > > > However, this does require that the start address of physical memory is
> > > > > a multiple of 128 MiB, precluding booting Linux on platforms where this
> > > > > requirement is not fulfilled.
> > > > >
> > > > > Fix this limitation by obtaining the start address from the DTB instead,
> > > > > if available (either explicitly passed, or appended to the kernel).
> > > > > Fall back to the traditional method when needed.
> > > > >
> > > > > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> > > > > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> > > > > i.e. not at a multiple of 128 MiB.
> > > > >
> > > > > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > ---
> > > >
> > > > [...]
> > > >
> > > > Apparently reading physical memory layout from DTB breaks crashdump
> > > > kernels. A crashdump kernel is loaded into a region of memory, that is
> > > > reserved in the original (i.e. to be crashed) kernel. The reserved
> > > > region is large enough for the crashdump kernel to run completely inside
> > > > it and don't modify anything outside it, just read and dump the remains
> > > > of the crashed kernel. Using the information from DTB makes the
> > > > decompressor place the kernel outside of the dedicated region.
> > > >
> > > > The log below shows that a zImage and DTB are loaded at 0x18eb8000 and
> > > > 0x193f6000 (physical). The kernel is expected to run at 0x18008000, but
> > > > it is decompressed to 0x00008000 (see r4 reported before jumping from
> > > > within __enter_kernel). If I were to suggest something, there need to be
> > > > one more bit of information passed in the DTB telling the decompressor
> > > > to use the old masking technique to determain kernel address. It would
> > > > be set in the DTB loaded along with the crashdump kernel.
> > >
> > > Shouldn't the DTB passed to the crashkernel describe which region of
> > > memory is to be used instead?
> >
> > Definitely not. The crashkernel needs to know where the RAM in the
> > machine is, so that it can create a coredump of the crashed kernel.
>
> So the DTB should describe both ;-)
>
> > > Describing "to use the old masking technique" sounds a bit hackish to me.
> > > I guess it cannot just restrict the /memory node to the reserved region,
> > > as the crashkernel needs to be able to dump the remains of the crashed
> > > kernel, which lie outside this region.
> >
> > Correct.
> >
> > > However, something under /chosen should work.
> >
> > Yet another sticky plaster...
>
> IMHO the old masking technique is the hacky solution covered by
> plasters.
One line of code is not "covered by plasters". There are no plasters.
It's a solution that works for 99.99% of people, unlike your approach
that has had a stream of issues over the last four months, and has
required many reworks of the code to fix each one. That in itself
speaks volumes about the suitability of the approach.
> DT describes the hardware.
Right, so DT is correct.
> In general, where to put the kernel is a
> software policy, and thus doesn't belong in DT, except perhaps under
> /chosen. But that would open another can of worms, as people usually
> have no business in specifying where the kernel should be located.
> In the crashkernel case, there is a clear separation between memory to
> be used by the crashkernel, and memory to be solely inspected by the
> crashkernel.
>
> Devicetree Specification, Release v0.3, Section 3.4 "/memory node" says:
>
> "The client program may access memory not covered by any memory
> reservations (see section 5.3)"
>
> (Section 5.3 "Memory Reservation Block" only talks about structures in
> the FDT, not about DTS)
>
> Hence according to the above, the crashkernel is rightfully allowed to
> do whatever it wants with all memory under the /memory node.
> However, there is also
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
> This suggests the crashkernel should be passed a DTB that contains a
> /reserved-memory node, describing which memory cannot be used freely.
> Then the decompressor needs to take this into account when deciding
> where the put the kernel.
So you want to throw yet more complexity at this solution to try and
make it work...
> Yes, the above requires changing code. But at least it provides a
> path forward, getting rid of the fragile old masking technique.
It's hardly fragile when it's worked fine for the last 20+ years,
whereas your solution can't work without some regression being reported
within a couple of weeks of it being applied. Again, that speaks
volumes about which one is better than the other.
Continually patching this approach to workaround one issue after another
shows that it is _this_ solution that is the fragile implementation.
A fragile implementation is by definition one that keeps breaking.
That's yours. The masking approach hasn't "broken" for anyone, and
hasn't been the cause of one single regression anywhere. Yes, there
are some platforms that it doesn't work for (because they choose to
reserve the first chunk of RAM for something) but that is not a
regression.
So, I'm not going to apply the next revision of this patch for at least
one whole kernel cycle - that means scheduling it for 5.10-rc at the
earliest.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn.
From: Will Deacon @ 2020-05-19 11:34 UTC (permalink / raw)
To: Dave Martin
Cc: Mark Rutland, Tamas Zsoldos, Vincenzo Frascino,
linux-arm-kernel@lists.infradead.org, Daniel Kiss
In-Reply-To: <20200519092934.GC5031@arm.com>
On Tue, May 19, 2020 at 10:29:37AM +0100, Dave Martin wrote:
> On Mon, May 18, 2020 at 05:00:32PM +0000, Daniel Kiss wrote:
> > >> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> > >> index 12324863d5c2..5d50ee92faa4 100644
> > >> --- a/arch/arm64/kernel/vdso/sigreturn.S
> > >> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> > >> @@ -13,13 +13,13 @@
> > >>
> > >> .text
> > >>
> > >> - nop
> > >> -SYM_FUNC_START(__kernel_rt_sigreturn)
> > >> .cfi_startproc
> > >> .cfi_signal_frame
> > >> .cfi_def_cfa x29, 0
> > >> .cfi_offset x29, 0 * 8
> > >> .cfi_offset x30, 1 * 8
> > >
> > > Hmm, recovering x29,x30 like this will be wrong if the signal handler
> > > munges sigcontext in the meantime (say, doing some kind of userspace
> > > context switch).
> > >
> > > They should be pulled out of sigcontext instead really. AFAIK, that's
> > > what ".cfi_signal_frame" is supposed to tell the unwinder. I'm not sure
> > > why we have these additional, conflicting annotations here.
I think we should just remove the .cfi_def_cfa and .cfi_offset directives
from this sequence. Daniel, Tamas, is that ok with you?
> > The unwinder won’t find the “cfi_signal_frame” until it figures out the unwind entry.
> >
> > > Any ideas, Will?
> > >
> > > This probably isn't related to the bug here, but it would be good to
> > > understand.
> > >
> > >> + nop /* placeholder for bl signalhandler */
> > >
> > > Will can correct me on this, but I seem to remember something about nop
> > > being there for padding, so that there is a guaranteed gap between
> > > unwind entries.
I think it is to do with unwinding, where something was subtracting 1
from the return address to identify the caller. There's an old relic here
that sheds a bit of light on it all (we inherited this from ppc, it seems):
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26208#c15
However, looking at gdb just now, it looks like it tries to match the
instructions in the trampoline in order to identify a signal frame. We just
changed that with the BTI patches, so I think it's now broken. Given that we
don't need the BTI C in there (we only call the thing via RET), I'll send a
patch to remove it.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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