Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
On Fri, Mar 08, 2024 at 05:18:21PM +0800, Andy Chiu wrote: > Hi Puranjay, > > On Fri, Mar 8, 2024 at 3:53 AM Puranjay Mohan wrote: > > > > Hi Björn, > > > > On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel wrote: > > > > > > Puranjay! > > > > > > Puranjay Mohan writes: > > > > > > > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V. > > > > This allows each ftrace callsite to provide an ftrace_ops to the common > > > > ftrace trampoline, allowing each callsite to invoke distinct tracer > > > > functions without the need to fall back to list processing or to > > > > allocate custom trampolines for each callsite. This significantly speeds > > > > up cases where multiple distinct trace functions are used and callsites > > > > are mostly traced by a single tracer. > > > > > > > > The idea and most of the implementation is taken from the ARM64's > > > > implementation of the same feature. The idea is to place a pointer to > > > > the ftrace_ops as a literal at a fixed offset from the function entry > > > > point, which can be recovered by the common ftrace trampoline. > > > > > > Not really a review, but some more background; Another rationale (on-top > > > of the improved per-call performance!) for CALL_OPS was to use it to > > > build ftrace direct call support (which BPF uses a lot!). Mark, please > > > correct me if I'm lying here! > > > > > > On Arm64, CALL_OPS makes it possible to implement direct calls, while > > > only patching one BL instruction -- nice! > > > > > > On RISC-V we cannot use use the same ideas as Arm64 straight off, > > > because the range of jal (compare to BL) is simply too short (+/-1M). > > > So, on RISC-V we need to use a full auipc/jal pair (the text patching > > > story is another chapter, but let's leave that aside for now). Since we > > > have to patch multiple instructions, the cmodx situation doesn't really > > > improve with CALL_OPS. > > > > > > Let's say that we continue building on your patch and implement direct > > > calls on CALL_OPS for RISC-V as well. > > > > > > From Florent's commit message for direct calls: > > > > > > |There are a few cases to distinguish: > > > |- If a direct call ops is the only one tracing a function: > > > | - If the direct called trampoline is within the reach of a BL > > > |instruction > > > | -> the ftrace patchsite jumps to the trampoline > > > | - Else > > > | -> the ftrace patchsite jumps to the ftrace_caller trampoline > > > which > > > |reads the ops pointer in the patchsite and jumps to the > > > direct > > > |call address stored in the ops > > > |- Else > > > | -> the ftrace patchsite jumps to the ftrace_caller trampoline > > > and its > > > | ops literal points to ftrace_list_ops so it iterates over all > > > | registered ftrace ops, including the direct call ops and > > > calls its > > > | call_direct_funcs handler which stores the direct called > > > | trampoline's address in the ftrace_regs and the ftrace_caller > > > | trampoline will return to that address instead of returning > > > to the > > > | traced function > > > > > > On RISC-V, where auipc/jalr is used, the direct called trampoline would > > > always be reachable, and then first Else-clause would never be entered. > > > This means the the performance for direct calls would be the same as the > > > one we have today (i.e. no regression!). > > > > > > RISC-V does like x86 does (-ish) -- patch multiple instructions, long > > > reach. > > > > > > Arm64 uses CALL_OPS and patch one instruction BL. > > > > > > Now, with this background in mind, compared to what we have today, > > > CALL_OPS would give us (again assuming we're using it for direct calls): > > > > > > * Better performance for tracer per-call (faster ops lookup) GOOD > > > > ^ this was the only motivation for me to implement this patch. > > > > I don't think implementing direct calls over call ops is fruitful for > > RISC-V because once > > the auipc/jalr can be patched atomically, the direct call trampoline > > is always reachable. > > Yes, the auipc/jalr instruction pair can be patched atomically just as > long as their size is naturally aligned on. However, we cannot prevent > 2 instructions from being fetched atomically :P There are some micro-arch optimization methods here, such as: - Disable interrupt when auipc retired. - When auipc -> auipc, the second one still could cause an interruption. > > > Solving the atomic text patching problem would be fun!! I am eager to > > see how it will be > > solved. > > I have a patch series to solve the atomic code patching issue, which I > am about to respin [1]. The idea is to solve it with yet another layer > of indirection. We add a 8-B aligned space at each function entry. The > space is a pointer to the ftrace entry. During boot, each function > entry code is updated to perform a
Re: [PATCH 2/4] csky: apply page shift to PFN instead of VA in pfn_to_virt
On Wed, Jan 31, 2024 at 2:28 PM Yan Zhao wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: c1884e1e1164 ("csky: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao
> ---
> arch/csky/include/asm/page.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
> index 4a0502e324a6..2c4cc7825a7b 100644
> --- a/arch/csky/include/asm/page.h
> +++ b/arch/csky/include/asm/page.h
> @@ -84,7 +84,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
>
> static inline void * pfn_to_virt(unsigned long pfn)
> {
> - return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
> + return __va(pfn << PAGE_SHIFT);
> }
Reviewed-by: Guo Ren
>
> #define MAP_NR(x) PFN_DOWN((unsigned long)(x) - PAGE_OFFSET - \
> --
> 2.17.1
>
--
Best Regards
Guo Ren
Re: [PATCH 1/4] asm-generic/page.h: apply page shift to PFN instead of VA in pfn_to_virt
On Wed, Jan 31, 2024 at 2:27 PM Yan Zhao wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao
> ---
> include/asm-generic/page.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/page.h b/include/asm-generic/page.h
> index 9773582fd96e..4f1265207b9a 100644
> --- a/include/asm-generic/page.h
> +++ b/include/asm-generic/page.h
> @@ -81,7 +81,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
> #define virt_to_pfn virt_to_pfn
> static inline void *pfn_to_virt(unsigned long pfn)
> {
> - return __va(pfn) << PAGE_SHIFT;
> + return __va(pfn << PAGE_SHIFT);
Oh, that's a terrible bug; Thx for fixing it.
Reviewed-by: Guo Ren
> }
> #define pfn_to_virt pfn_to_virt
>
> --
> 2.17.1
>
--
Best Regards
Guo Ren
Re: [RFC PATCH 11/14] csky/thread_info: Introduce TIF_NOTIFY_IPI flag
On Wed, Feb 21, 2024 at 1:20 AM K Prateek Nayak wrote: > > Add support for TIF_NOTIFY_IPI on C-SKY. With TIF_NOTIFY_IPI, a sender > sending an IPI to an idle CPU in TIF_POLLING mode will set the > TIF_NOTIFY_IPI flag in the target's idle tasks's thread_info to pull the > CPU out of idle, as opposed to setting TIF_NEED_RESCHED previously. This > avoids spurious calls to schedule_idle() in cases where an IPI does not > necessarily wake up a task on the idle CPU. > > Cc: Guo Ren > Cc: "Rafael J. Wysocki" > Cc: Daniel Lezcano > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Juri Lelli > Cc: Vincent Guittot > Cc: Dietmar Eggemann > Cc: Steven Rostedt > Cc: Ben Segall > Cc: Mel Gorman > Cc: Daniel Bristot de Oliveira > Cc: Valentin Schneider > Cc: K Prateek Nayak > Cc: linux-c...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux...@vger.kernel.org > Signed-off-by: K Prateek Nayak > --- > arch/csky/include/asm/thread_info.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/csky/include/asm/thread_info.h > b/arch/csky/include/asm/thread_info.h > index b5ed788f0c68..9bc7a037c476 100644 > --- a/arch/csky/include/asm/thread_info.h > +++ b/arch/csky/include/asm/thread_info.h > @@ -61,6 +61,7 @@ static inline struct thread_info *current_thread_info(void) > #define TIF_SYSCALL_TRACEPOINT 5 /* syscall tracepoint instrumentation > */ > #define TIF_SYSCALL_AUDIT 6 /* syscall auditing */ > #define TIF_NOTIFY_SIGNAL 7 /* signal notifications exist */ > +#define TIF_NOTIFY_IPI 8 /* Pending IPI on TIF_POLLLING idle > CPU */ > #define TIF_POLLING_NRFLAG 16 /* poll_idle() is TIF_NEED_RESCHED */ > #define TIF_MEMDIE 18 /* is terminating due to OOM killer */ > #define TIF_RESTORE_SIGMASK20 /* restore signal mask in do_signal() > */ > @@ -73,6 +74,7 @@ static inline struct thread_info *current_thread_info(void) > #define _TIF_SYSCALL_TRACEPOINT(1 << TIF_SYSCALL_TRACEPOINT) > #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) > #define _TIF_NOTIFY_SIGNAL (1 << TIF_NOTIFY_SIGNAL) > +#define _TIF_NOTIFY_IPI(1 << TIF_NOTIFY_IPI) > #define _TIF_UPROBE(1 << TIF_UPROBE) > #define _TIF_POLLING_NRFLAG(1 << TIF_POLLING_NRFLAG) > #define _TIF_MEMDIE(1 << TIF_MEMDIE) > -- > 2.34.1 > LGTM Acked-by: Guo Ren -- Best Regards Guo Ren
Re: [PATCH 10/10] csky/vdso: Use generic union vdso_data_store
On Mon, Feb 19, 2024 at 11:40 PM Anna-Maria Behnsen
wrote:
>
> There is already a generic union definition for vdso_data_store in vdso
> datapage header.
>
> Use this definition to prevent code duplication.
>
> Signed-off-by: Anna-Maria Behnsen
> Cc: Guo Ren
> Cc: linux-c...@vger.kernel.org
> ---
> arch/csky/kernel/vdso.c | 10 ++
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/csky/kernel/vdso.c b/arch/csky/kernel/vdso.c
> index e74a2504d331..2ca886e4a458 100644
> --- a/arch/csky/kernel/vdso.c
> +++ b/arch/csky/kernel/vdso.c
> @@ -15,14 +15,8 @@ extern char vdso_start[], vdso_end[];
> static unsigned int vdso_pages;
> static struct page **vdso_pagelist;
>
> -/*
> - * The vDSO data page.
> - */
> -static union {
> - struct vdso_datadata;
> - u8 page[PAGE_SIZE];
> -} vdso_data_store __page_aligned_data;
> -struct vdso_data *vdso_data = _data_store.data;
> +static union vdso_data_store vdso_data_store __page_aligned_data;
> +struct vdso_data *vdso_data = vdso_data_store.data;
>
> static int __init vdso_init(void)
> {
> --
> 2.39.2
>
Acked-by: Guo Ren
--
Best Regards
Guo Ren
Re: [PATCH 03/10] csky/vdso: Remove superfluous ifdeffery
On Mon, Feb 19, 2024 at 11:40 PM Anna-Maria Behnsen
wrote:
>
> CSKY selects GENERIC_TIME_VSYSCALL. GENERIC_TIME_VSYSCALL dependent
> ifdeffery is superfluous. Clean it up.
>
> Signed-off-by: Anna-Maria Behnsen
> Cc: Guo Ren
> Cc: linux-c...@vger.kernel.org
> ---
> arch/csky/include/asm/vdso.h | 5 -
> arch/csky/kernel/vdso.c | 4
> 2 files changed, 9 deletions(-)
>
> diff --git a/arch/csky/include/asm/vdso.h b/arch/csky/include/asm/vdso.h
> index bdce581b5fcb..181a15edafe8 100644
> --- a/arch/csky/include/asm/vdso.h
> +++ b/arch/csky/include/asm/vdso.h
> @@ -5,11 +5,6 @@
>
> #include
>
> -#ifndef GENERIC_TIME_VSYSCALL
> -struct vdso_data {
> -};
> -#endif
> -
> /*
> * The VDSO symbols are mapped into Linux so we can just use regular symbol
> * addressing to get their offsets in userspace. The symbols are mapped at
> an
> diff --git a/arch/csky/kernel/vdso.c b/arch/csky/kernel/vdso.c
> index 16c20d64d165..e74a2504d331 100644
> --- a/arch/csky/kernel/vdso.c
> +++ b/arch/csky/kernel/vdso.c
> @@ -8,11 +8,7 @@
> #include
>
> #include
> -#ifdef GENERIC_TIME_VSYSCALL
> #include
> -#else
> -#include
> -#endif
>
> extern char vdso_start[], vdso_end[];
>
> --
> 2.39.2
>
Acked-by: Guo Ren
--
Best Regards
Guo Ren
Re: [PATCH V12 00/14] riscv: Add Native/Paravirt qspinlock support
Sorry, I missed people on the list. F.Y.I Waiman Long Boqun Feng Here is Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guo...@kernel.org/ On Mon, Dec 25, 2023 at 8:59 PM wrote: > > From: Guo Ren > > patch[1 - 8]: Native qspinlock > patch[9 -14]: Paravirt qspinlock > > This series based on: > - v6.7-rc7 > - Rework & improve riscv cmpxchg.h and atomic.h > > https://lore.kernel.org/linux-riscv/20230810040349.92279-2-leob...@redhat.com/ > > You can directly try it: > https://github.com/guoren83/linux/tree/qspinlock_v12 > > Native qspinlock > > > This time we've proved the qspinlock on th1520 [1] & sg2042 [2], which > gives stability and performance improvement. All T-HEAD processors have > a strong LR/SC forward progress guarantee than the requirements of the > ISA, which could satisfy the xchg_tail of native_qspinlock. Now, > qspinlock has been run with us for more than 1 year, and we have enough > confidence to enable it for all the T-HEAD processors. Of causes, we > found a livelock problem with the qspinlock lock torture test from the > CPU store merge buffer delay mechanism, which caused the queued spinlock > becomes a dead ring and RCU warning to come out. We introduce a custom > WRITE_ONCE to solve this, which will be fixed in the next generation of > hardware. > > We've tested the patch on SOPHGO sg2042 & th1520 and passed the stress > test on Fedora & Ubuntu & OpenEuler ... Here is the performance > comparison between qspinlock and ticket_lock on sg2042 (64 cores): > > sysbench test=threads threads=32 yields=100 lock=8 (+13.8%): > queued_spinlock 0.5109/0.00 > ticket_spinlock 0.5814/0.00 > > perf futex/hash (+6.7%): > queued_spinlock 1444393 operations/sec (+- 0.09%) > ticket_spinlock 1353215 operations/sec (+- 0.15%) > > perf futex/wake-parallel (+8.6%): > queued_spinlock (waking 1/64 threads) in 0.0253 ms (+-2.90%) > ticket_spinlock (waking 1/64 threads) in 0.0275 ms (+-3.12%) > > perf futex/requeue (+4.2%): > queued_spinlock Requeued 64 of 64 threads in 0.0785 ms (+-0.55%) > ticket_spinlock Requeued 64 of 64 threads in 0.0818 ms (+-4.12%) > > > System Benchmarks (+6.4%) > queued_spinlock: > System Benchmarks Index Values BASELINE RESULT > INDEX > Dhrystone 2 using register variables 116700.0 628613745.4 > 53865.8 > Double-Precision Whetstone 55.0 182422.8 > 33167.8 > Execl Throughput 43.0 13116.6 > 3050.4 > File Copy 1024 bufsize 2000 maxblocks 3960.07762306.2 > 19601.8 > File Copy 256 bufsize 500 maxblocks1655.03417556.8 > 20649.9 > File Copy 4096 bufsize 8000 maxblocks 5800.07427995.7 > 12806.9 > Pipe Throughput 12440.0 23058600.5 > 18535.9 > Pipe-based Context Switching 4000.02835617.7 > 7089.0 > Process Creation126.0 12537.3 > 995.0 > Shell Scripts (1 concurrent) 42.4 57057.4 > 13456.9 > Shell Scripts (8 concurrent) 6.0 7367.1 > 12278.5 > System Call Overhead 15000.0 33308301.3 > 22205.5 > > > System Benchmarks Index Score > 12426.1 > > ticket_spinlock: > System Benchmarks Index Values BASELINE RESULT > INDEX > Dhrystone 2 using register variables 116700.0 626541701.9 > 53688.2 > Double-Precision Whetstone 55.0 181921.0 > 33076.5 > Execl Throughput 43.0 12625.1 > 2936.1 > File Copy 1024 bufsize 2000 maxblocks 3960.06553792.9 > 16550.0 > File Copy 256 bufsize 500 maxblocks1655.03189231.6 > 19270.3 > File Copy 4096 bufsize 8000 maxblocks 5800.07221277.0 > 12450.5 > Pipe Throughput 12440.0 20594018.7 > 16554.7 > Pipe-based Context Switching 4000.02571117.7 > 6427.8 > Process Creation126.0 10798.4 > 857.0 > Shell Scripts (1 concurrent) 42.4 57227.5 > 13497.1 > Shell Scripts (8 concurrent) 6.0 7329.2 > 12215.3 > System Call Overhead 15000.0 30766778.4 > 20511.2 >
Re: [PATCH] set_thread_area.2: Add C-SKY document
On Mon, Oct 16, 2023 at 2:00 AM Alejandro Colomar wrote: > > Hi Guo, > > On Sun, Oct 15, 2023 at 11:07:32AM -0400, guo...@kernel.org wrote: > > From: Guo Ren > > > > C-SKY only needs set_thread_area, no need for get_thread_area, the > > same as MIPS. > > > > Signed-off-by: Guo Ren > > Signed-off-by: Guo Ren > > --- > > Patch applied. > <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib=1c4464ae2c40318b77e125961e24710d1784df5d> > > Thanks! > Alex > > > man2/set_thread_area.2 | 10 +- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/man2/set_thread_area.2 b/man2/set_thread_area.2 > > index 02f65e0418f2..c43a92eb447a 100644 > > --- a/man2/set_thread_area.2 > > +++ b/man2/set_thread_area.2 > > @@ -26,7 +26,7 @@ Standard C library > > .B "int syscall(SYS_get_thread_area);" > > .BI "int syscall(SYS_set_thread_area, unsigned long " tp ); > > .PP > > -.B #elif defined __mips__ > > +.B #elif defined(__mips__ || defined __csky__) > > I removed the parentheses here. Oops, thanks. > > > .PP > > .BI "int syscall(SYS_set_thread_area, unsigned long " addr ); > > .PP > > @@ -42,17 +42,17 @@ These calls provide architecture-specific support for a > > thread-local storage > > implementation. > > At the moment, > > .BR set_thread_area () > > -is available on m68k, MIPS, and x86 (both 32-bit and 64-bit variants); > > +is available on m68k, MIPS, C-SKY, and x86 (both 32-bit and 64-bit > > variants); > > .BR get_thread_area () > > is available on m68k and x86. > > .PP > > -On m68k and MIPS, > > +On m68k, MIPS and C-SKY, > > .BR set_thread_area () > > allows storing an arbitrary pointer (provided in the > > .B tp > > argument on m68k and in the > > .B addr > > -argument on MIPS) > > +argument on MIPS and C-SKY) > > in the kernel data structure associated with the calling thread; > > this pointer can later be retrieved using > > .BR get_thread_area () > > @@ -139,7 +139,7 @@ return 0 on success, and \-1 on failure, with > > .I errno > > set to indicate the error. > > .PP > > -On MIPS and m68k, > > +On C-SKY, MIPS and m68k, > > .BR set_thread_area () > > always returns 0. > > On m68k, > > -- > > 2.36.1 > > > > -- > <https://www.alejandro-colomar.es/> -- Best Regards Guo Ren
Re: set_thread_area.2: csky architecture undocumented
On Sun, Oct 15, 2023 at 01:20:42AM +0200, Alejandro Colomar wrote:
> Hi Guo,
>
> On Tue, Nov 24, 2020 at 08:07:07PM +0800, Guo Ren wrote:
>
> Huh, 3 years already! I've had this in my head for all this time; just
> didn't find the energy to act on it.
>
> > Thx Michael & Alejandro,
> >
> > Yes, the man page has no csky's.
>
> I've applied a patch to add initial documentation for it:
> <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib=a63979eb24aaf73f4be5799cc9579f84a3874b7d>
>
> >
> > C-SKY have abiv1 and abiv2
> > For abiv1: There is no register for tls saving, We use trap 3 to got
> > tls and use set_thread_area to init ti->tp_value.
> > For abiv2: The r31 is the tls register. We could directly read r31 to
> > got r31 and use set_thread_area to init reg->tls value.
> >
> > In glibc:
> > # ifdef __CSKYABIV2__
> > /* Define r31 as thread pointer register. */
> > # define READ_THREAD_POINTER() \
> > mov r0, r31;
> > # else
> > # define READ_THREAD_POINTER() \
> > trap 3;
> > # endif
> >
> > /* Code to initially initialize the thread pointer. This might need
> >special attention since 'errno' is not yet available and if the
> >operation can cause a failure 'errno' must not be touched. */
> > # define TLS_INIT_TP(tcbp) \
> > ({ INTERNAL_SYSCALL_DECL (err); \
> > long result_var; \
> > result_var = INTERNAL_SYSCALL (set_thread_area, err, 1,\
> > (char *) (tcbp) + TLS_TCB_OFFSET); \
> > INTERNAL_SYSCALL_ERROR_P (result_var, err) \
> >? "unknown error" : NULL; })
> >
> > In kernel:
> > SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
> > {
> > struct thread_info *ti = task_thread_info(current);
> > struct pt_regs *reg = current_pt_regs();
> >
> > reg->tls = addr;
> > ti->tp_value = addr;
> >
> > return 0;
> > }
> >
> > Any comments are welcome :)
>
> I'm sorry, but I have little understanding of this syscall, and that
> shounds like gibberish to me :)
>
> Feel free to send a patch to improve the documentation for csky.
Yeah, I've sent a patch for it; please review:
https://lore.kernel.org/linux-csky/20231015150732.1991997-1-guo...@kernel.org/
>
> Cheers,
> Alex
>
> >
> >
> > On Tue, Nov 24, 2020 at 5:51 PM Michael Kerrisk (man-pages)
> > wrote:
> > >
> > > Hi Alex,
> > >
> > > On 11/23/20 10:31 PM, Alejandro Colomar (man-pages) wrote:
> > > > Hi Michael,
> > > >
> > > > SYNOPSIS
> > > >#include
> > > >
> > > >#if defined __i386__ || defined __x86_64__
> > > ># include
> > > >
> > > >int get_thread_area(struct user_desc *u_info);
> > > >int set_thread_area(struct user_desc *u_info);
> > > >
> > > >#elif defined __m68k__
> > > >
> > > >int get_thread_area(void);
> > > >int set_thread_area(unsigned long tp);
> > > >
> > > >#elif defined __mips__
> > > >
> > > >int set_thread_area(unsigned long addr);
> > > >
> > > >#endif
> > > >
> > > >Note: There are no glibc wrappers for these system calls; see
> > > >NOTES.
> > > >
> > > >
> > > > $ grep -rn 'SYSCALL_DEFINE.*et_thread_area'
> > > > arch/csky/kernel/syscall.c:6:
> > > > SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
> > > > arch/mips/kernel/syscall.c:86:
> > > > SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
> > > > arch/x86/kernel/tls.c:191:
> > > > SYSCALL_DEFINE1(set_thread_area, struct user_desc __user *, u_info)
> > > > arch/x86/kernel/tls.c:243:
> > > > SYSCALL_DEFINE1(get_thread_area, struct user_desc __user *, u_info)
> > > > arch/x86/um/tls_32.c:277:
> > > > SYSCALL_DEFINE1(set_thread_area, struct user_desc __user *, user_desc)
> > > > arch/x86/um/tls_32.c:325:
> > > > SYSCALL_DEFINE1(get_thread_area, struct user_desc __user *, user_desc)
> > > >
> > > >
> > > > See kernel commit 4859bfca11c7d63d55175bcd85a75d6cee4b7184
> > > >
> > > >
> > > > I'd change
> > > > - #elif defined __mips__
> > > > + #elif defined(__mips__ || __csky__)
> > > >
> > > > and then change the rest of the text to add csky when appropriate.
> > > > Am I correct?
> > >
> > > AFAICT, you are correct. I think the reason that csky is missing is
> > > that the architecture was added after this manual pages was added.
> > >
> > > Thanks,
> > >
> > > Michael
> > >
> > >
> > > --
> > > Michael Kerrisk
> > > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> > > Linux/UNIX System Programming Training: http://man7.org/training/
> >
> >
> >
> > --
> > Best Regards
> > Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/
>
> --
> <https://www.alejandro-colomar.es/>
Re: [PATCH 4/5] kbuild: unify vdso_install rules
On Wed, Oct 11, 2023 at 8:53 PM Masahiro Yamada wrote: > > On Wed, Oct 11, 2023 at 11:24 AM Guo Ren wrote: > > > > On Mon, Oct 9, 2023 at 8:42 PM Masahiro Yamada wrote: > > > > --- a/arch/riscv/Makefile > > > +++ b/arch/riscv/Makefile > > > @@ -131,12 +131,6 @@ endif > > > libs-y += arch/riscv/lib/ > > > libs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a > > > > > > -PHONY += vdso_install > > > -vdso_install: > > > - $(Q)$(MAKE) $(build)=arch/riscv/kernel/vdso $@ > > > - $(if $(CONFIG_COMPAT),$(Q)$(MAKE) \ > > > - $(build)=arch/riscv/kernel/compat_vdso compat_$@) > > > - > > > ifeq ($(KBUILD_EXTMOD),) > > > ifeq ($(CONFIG_MMU),y) > > > prepare: vdso_prepare > > > @@ -148,6 +142,9 @@ vdso_prepare: prepare0 > > > endif > > > endif > > > > > > +vdso-install-y += arch/riscv/kernel/vdso/vdso.so.dbg > > > +vdso-install-$(CONFIG_COMPAT) += > > > arch/riscv/kernel/compat_vdso/compat_vdso.so.dbg:../compat_vdso/compat_vdso.so > > Why do we need ":../compat_vdso/compat_vdso.so" here? > > > > > All architectures except riscv install vdso files > to /lib/modules/$(uname -r)/vdso/. > > > > See the following code in arch/riscv/kernel/compat_vdso/Makefile: > > > quiet_cmd_compat_vdso_install = INSTALL $@ > cmd_compat_vdso_install = cp $(obj)/$@.dbg $(MODLIB)/compat_vdso/$@ > > > > > Riscv copies the compat vdso to > /lib/modules/$(uname -r)/compat_vdso/. > > > > This commit preserves the current installation path as-is. > > If the riscv maintainers agree, we can change the > installation destination to /lib/modules/$(uname -r)/vdso/ > for consistency. Yes, but it should be another patch. Thx for the clarification. Reviewed-by: Guo Ren > > > > -- > Best Regards > Masahiro Yamada -- Best Regards Guo Ren
Re: [PATCH 4/5] kbuild: unify vdso_install rules
-# > -# Install the unstripped copies of vdso*.so. If our toolchain supports > -# build-id, install .build-id links as well. > -# > -quiet_cmd_vdso_install = INSTALL $(@:install_%=%) > -define cmd_vdso_install > - cp $< "$(MODLIB)/vdso/$(@:install_%=%)"; \ > - if readelf -n $< |grep -q 'Build ID'; then \ > - buildid=`readelf -n $< |grep 'Build ID' |sed -e 's/^.*Build ID: > \(.*\)$$/\1/'`; \ > - first=`echo $$buildid | cut -b-2`; \ > - last=`echo $$buildid | cut -b3-`; \ > - mkdir -p "$(MODLIB)/vdso/.build-id/$$first"; \ > - ln -sf "../../$(@:install_%=%)" > "$(MODLIB)/vdso/.build-id/$$first/$$last.debug"; \ > - fi > -endef > - > -vdso_img_insttargets := $(vdso_img_sodbg:%.dbg=install_%) > - > -$(MODLIB)/vdso: FORCE > - @mkdir -p $(MODLIB)/vdso > - > -$(vdso_img_insttargets): install_%: $(obj)/%.dbg $(MODLIB)/vdso > - $(call cmd,vdso_install) > - > -PHONY += vdso_install $(vdso_img_insttargets) > -vdso_install: $(vdso_img_insttargets) > - > clean-files := vdso32.so vdso32.so.dbg vdso64* vdso-image-*.c vdsox32.so* > diff --git a/scripts/Makefile.vdsoinst b/scripts/Makefile.vdsoinst > new file mode 100644 > index ..1022d9fdd976 > --- /dev/null > +++ b/scripts/Makefile.vdsoinst > @@ -0,0 +1,45 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# == > +# Install unstripped copies of vDSO > +# == > + > +PHONY := __default > +__default: > + @: > + > +include $(srctree)/scripts/Kbuild.include > + > +install-dir := $(MODLIB)/vdso > + > +define gen_install_rules > + > +src := $$(firstword $$(subst :,$(space),$(1))) > +dest := $(install-dir)/$$(or $$(word 2,$$(subst > :,$(space),$(1))),$$(patsubst %.dbg,%,$$(notdir $(1 > + > +__default: $$(dest) > +$$(dest): $$(src) FORCE > + $$(call cmd,install) > + > +# Some architectures create .build-id symlinks > +ifneq ($(filter arm sparc x86, $(SRCARCH)),) > +link := $(install-dir)/.build-id/$$(shell $(READELF) -n $$(src) | sed -n > 's@^.*Build ID: \(..\)\(.*\)@\1/\2@p') > + > +__default: $$(link) > +$$(link): $$(dest) FORCE > + $$(call cmd,symlink) > +endif > + > +endef > + > +$(foreach x, $(sort $(INSTALL_FILES)), $(eval $(call > gen_install_rules,$(x > + > +quiet_cmd_install = INSTALL $@ > + cmd_install = mkdir -p $(dir $@); cp $< $@ > + > +quiet_cmd_symlink = SYMLINK $@ > + cmd_symlink = mkdir -p $(dir $@); ln -sf --relative $< $@ > + > +PHONY += FORCE > +FORCE: > + > +.PHONY: $(PHONY) > -- > 2.39.2 > -- Best Regards Guo Ren
Re: [PATCH 1/5] csky: remove unused cmd_vdso_install
On Mon, Oct 9, 2023 at 8:42 PM Masahiro Yamada wrote: > > You cannot run this code because arch/csky/Makefile does not define the > vdso_install target. > > It appears that this code was blindly copied from another architecture. Yes, I do that. Thx for pointing it out. Acked-by: Guo Ren > > Remove the dead code. > > Signed-off-by: Masahiro Yamada > --- > > arch/csky/kernel/vdso/Makefile | 10 -- > 1 file changed, 10 deletions(-) > > diff --git a/arch/csky/kernel/vdso/Makefile b/arch/csky/kernel/vdso/Makefile > index 299e4e41ebc5..ddf784a62c11 100644 > --- a/arch/csky/kernel/vdso/Makefile > +++ b/arch/csky/kernel/vdso/Makefile > @@ -58,13 +58,3 @@ quiet_cmd_vdsold = VDSOLD $@ > # that contains the same symbols at the same offsets. > quiet_cmd_so2s = SO2S$@ >cmd_so2s = $(NM) -D $< | $(srctree)/$(src)/so2s.sh > $@ > - > -# install commands for the unstripped file > -quiet_cmd_vdso_install = INSTALL $@ > - cmd_vdso_install = cp $(obj)/$@.dbg $(MODLIB)/vdso/$@ > - > -vdso.so: $(obj)/vdso.so.dbg > - @mkdir -p $(MODLIB)/vdso > - $(call cmd,vdso_install) > - > -vdso_install: vdso.so > -- > 2.39.2 > -- Best Regards Guo Ren
Re: [PATCH v2 (RESEND) 2/2] riscv: atomic: Using ARCH_ATOMIC in asm/atomic.h
On Mon, Apr 19, 2021 at 7:46 PM Arnd Bergmann wrote: > > On Sat, Apr 17, 2021 at 6:45 AM wrote: > > +#define arch_atomic_read(v)__READ_ONCE((v)->counter) > > +#define arch_atomic_set(v, i) > > __WRITE_ONCE(((v)->counter), (i)) > > > +#define ATOMIC64_INIT ATOMIC_INIT > > +#define arch_atomic64_read arch_atomic_read > > +#define arch_atomic64_set arch_atomic_set > > #endif > > I think it's a bit confusing to define arch_atomic64_read() etc in terms > of arch_atomic_read(), given that they operate on different types. > > IMHO the clearest would be to define both in terms of the open-coded > version you have for the 32-bit atomics. Okay: +#define arch_atomic64_read __READ_ONCE((v)->counter) +#define arch_atomic64_set __WRITE_ONCE(((v)->counter), (i)) > > Also, given that all three architectures (x86, arm64, riscv) use the same > definitions for the six macros above, maybe those can just get moved > into a common file with a possible override? I'll try it with a separate patch. > > x86 uses an inline function here instead of the macro. This would also > be my preference, but it may add complexity to avoid circular header > dependencies. > > The rest of this patch looks good to me. > > Arnd -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v2 1/2] locking/atomics: Fixup GENERIC_ATOMIC64 conflict with atomic-arch-fallback.h
Abandoned, it has duplicated definition export in gen-atomic-instrumented.sh
On Sat, Apr 17, 2021 at 10:57 AM wrote:
>
> From: Guo Ren
>
> Current GENERIC_ATOMIC64 in atomic-arch-fallback.h is broken. When a 32-bit
> arch use atomic-arch-fallback.h will cause compile error.
>
> In file included from include/linux/atomic.h:81,
> from include/linux/rcupdate.h:25,
> from include/linux/rculist.h:11,
> from include/linux/pid.h:5,
> from include/linux/sched.h:14,
> from arch/riscv/kernel/asm-offsets.c:10:
>include/linux/atomic-arch-fallback.h: In function 'arch_atomic64_inc':
> >> include/linux/atomic-arch-fallback.h:1447:2: error: implicit declaration
> >> of function 'arch_atomic64_add'; did you mean 'arch_atomic_add'?
> >> [-Werror=implicit-function-declaration]
> 1447 | arch_atomic64_add(1, v);
> | ^
> | arch_atomic_add
>
> The atomic-arch-fallback.h & atomic-fallback.h &
> atomic-instrumented.h are generated by gen-atomic-fallback.sh &
> gen-atomic-instrumented.sh, so just take care the bash files.
>
> Remove the dependency of atomic-*-fallback.h in atomic64.h.
>
> Signed-off-by: Guo Ren
> Cc: Peter Zijlstra
> Cc: Arnd Bergmann
> ---
> include/asm-generic/atomic-instrumented.h | 307
> +-
> include/asm-generic/atomic64.h| 89 +
> include/linux/atomic-arch-fallback.h | 5 +-
> include/linux/atomic-fallback.h | 5 +-
> scripts/atomic/gen-atomic-fallback.sh | 3 +-
> scripts/atomic/gen-atomic-instrumented.sh | 23 ++-
> 6 files changed, 294 insertions(+), 138 deletions(-)
>
> diff --git a/include/asm-generic/atomic-instrumented.h
> b/include/asm-generic/atomic-instrumented.h
> index 888b6cf..f6ce7a2 100644
> --- a/include/asm-generic/atomic-instrumented.h
> +++ b/include/asm-generic/atomic-instrumented.h
> @@ -831,6 +831,180 @@ atomic_dec_if_positive(atomic_t *v)
> #define atomic_dec_if_positive atomic_dec_if_positive
> #endif
>
> +#if !defined(arch_xchg_relaxed) || defined(arch_xchg)
> +#define xchg(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_xchg(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_xchg_acquire)
> +#define xchg_acquire(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_xchg_acquire(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_xchg_release)
> +#define xchg_release(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_xchg_release(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_xchg_relaxed)
> +#define xchg_relaxed(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_xchg_relaxed(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if !defined(arch_cmpxchg_relaxed) || defined(arch_cmpxchg)
> +#define cmpxchg(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_cmpxchg_acquire)
> +#define cmpxchg_acquire(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_cmpxchg_acquire(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_cmpxchg_release)
> +#define cmpxchg_release(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_cmpxchg_release(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_cmpxchg_relaxed)
> +#define cmpxchg_relaxed(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_cmpxchg_relaxed(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if !defined(arch_cmpxchg64_relaxed) || defined(arch_cmpxchg64)
> +#define cmpxchg64(ptr, ...) \
> +({ \
> + typeof(ptr) __ai_ptr = (ptr); \
> + instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
> + arch_cmpxchg64(__ai_ptr, __VA_ARGS__); \
> +})
> +#endif
> +
> +#if defined(arch_cmpxchg64_acquire)
> +
Re: [PATCH] riscv: atomic: Using ARCH_ATOMIC in asm/atomic.h
On Thu, Apr 15, 2021 at 4:52 PM Peter Zijlstra wrote: > > On Thu, Apr 15, 2021 at 07:39:22AM +, guo...@kernel.org wrote: > > - Add atomic_andnot_* operation > > > @@ -76,6 +59,12 @@ ATOMIC_OPS(sub, add, -i) > > ATOMIC_OPS(and, and, i) > > ATOMIC_OPS( or, or, i) > > ATOMIC_OPS(xor, xor, i) > > +ATOMIC_OPS(andnot, and, -i) > > ~i, surely. Thx for correct me. I'll fix it in the next version patch. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: atomic: Using ARCH_ATOMIC in asm/atomic.h
Sorry, forgot riscv32. I would fix that. On Thu, Apr 15, 2021 at 4:52 PM Peter Zijlstra wrote: > > On Thu, Apr 15, 2021 at 07:39:22AM +, guo...@kernel.org wrote: > > - Add atomic_andnot_* operation > > > @@ -76,6 +59,12 @@ ATOMIC_OPS(sub, add, -i) > > ATOMIC_OPS(and, and, i) > > ATOMIC_OPS( or, or, i) > > ATOMIC_OPS(xor, xor, i) > > +ATOMIC_OPS(andnot, and, -i) > > ~i, surely. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [RFC][PATCH] locking: Generic ticket-lock
read(lock);
> +
> + return (s16)((val >> 16) - (val & 0x)) > 1;
How big-endian ?
return (tickets.next - tickets.owner) > 1;
> +}
> +
> +static __always_inline int ticket_value_unlocked(arch_spinlock_t lock)
> +{
> + return !ticket_is_locked();
Are you sure to let ticket_is_locked->atomic_read(lock) again, the
lock has contained all information?
return lock.tickets.owner == lock.tickets.next;
> +}
> +
> +#define arch_spin_lock(l) ticket_lock(l)
> +#define arch_spin_trylock(l) ticket_trylock(l)
> +#define arch_spin_unlock(l)ticket_unlock(l)
> +#define arch_spin_is_locked(l) ticket_is_locked(l)
> +#define arch_spin_is_contended(l) ticket_is_contended(l)
> +#define arch_spin_value_unlocked(l)ticket_value_unlocked(l)
> +
> +#endif /* __ASM_GENERIC_TICKET_LOCK_H */
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
On Tue, Apr 13, 2021 at 6:54 PM David Laight wrote:
>
> From: Catalin Marinas
> > Sent: 13 April 2021 11:45
> ...
> > This indeed needs some care. IIUC RISC-V has similar restrictions as arm
> > here, no load/store instructions are allowed between LR and SC. You
> > can't guarantee that the compiler won't spill some variable onto the
> > stack.
>
> You can probably never guarantee the compiler won't spill to stack.
> Especially if someone compiles with -O0.
>
> Which probably means that anything using LR/SC must be written in
> asm and the C wrappers disabled.
Agree, and cmpxchg has been widely used in Linux. I think it's the
last requirement for complex atomic API, although cmpxchg has ABA
problem:
CPU0
CPU1
===
==
do {
old32 = load32;
*ptr32 = new32_tmp;
*ptr32 = old32;
load32 = cmpxchg(ptr32, old32, new32); //still success
} while (load32 != old32);
That means cmpxhg only cares about the result but not the middle
situation. It's different from LR/SC or AMO instructions.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
> 1PT, UK
> Registration No: 1397386 (Wales)
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
Thx Peter,
On Tue, Apr 13, 2021 at 4:17 PM Peter Zijlstra wrote:
>
> On Tue, Apr 13, 2021 at 10:03:01AM +0200, Peter Zijlstra wrote:
>
> > For ticket locks you really only needs atomic_fetch_add() and
> > smp_store_release() and an architectural guarantees that the
> > atomic_fetch_add() has fwd progress under contention and that a sub-word
> > store (through smp_store_release()) will fail the SC.
> >
> > Then you can do something like:
> >
> > void lock(atomic_t *lock)
> > {
> > u32 val = atomic_fetch_add(1<<16, lock); /* SC, gives us RCsc */
> > u16 ticket = val >> 16;
> >
> > for (;;) {
> > if (ticket == (u16)val)
> > break;
> > cpu_relax();
> > val = atomic_read_acquire(lock);
> > }
Should it be?
for (;;) {
if (ticket == (u16)val) {
__atomic_acquire_fence();
break;
}
>
> A possibly better might be:
>
> if (ticket == (u16)val)
> return;
Should it be?
if (ticket == (u16)val) {
__atomic_acquire_fence();
return;
}
>
> atomic_cond_read_acquire(lock, ticket == (u16)VAL);
>
> Since that allows architectures to use WFE like constructs.
>
> > }
> >
> > void unlock(atomic_t *lock)
> > {
> > u16 *ptr = (u16 *)lock + (!!__BIG_ENDIAN__);
> > u32 val = atomic_read(lock);
> >
> > smp_store_release(ptr, (u16)val + 1);
> > }
> >
> > That's _almost_ as simple as a test-and-set :-) It isn't quite optimal
> > on x86 for not being allowed to use a memop on unlock, since its being
> > forced into a load-store because of all the volatile, but whatever.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
e.
That means the HW implementation should use a per-hart address monitor
not just grab the cache line into the exclusive state without lockdown
the SNOOP channel.
I think the implementation of LR/SC you mentioned is a gambling style
but broke the riscv spec.
Is the patch of Will's would fix up the problem you mentioned?
commit 9bb17be062de6f5a9c9643258951aa0935652ec3
Author: Will Deacon
Date: Tue Jul 2 14:54:33 2013 +0100
ARM: locks: prefetch the destination word for write prior to strex
The cost of changing a cacheline from shared to exclusive state can be
significant, especially when this is triggered by an exclusive store,
since it may result in having to retry the transaction.
This patch prefixes our {spin,read,write}_[try]lock implementations with
pldw instructions (on CPUs which support them) to try and grab the line
in exclusive state from the start. arch_rwlock_t is changed to avoid
using a volatile member, since this generates compiler warnings when
falling back on the __builtin_prefetch intrinsic which expects a const
void * argument.
Acked-by: Nicolas Pitre
Signed-off-by: Will Deacon
In the end, I want to conclude my suggestions here:
- Using ticket-lock as default
- Using ARCH_USE_QUEUED_SPINLOCKS_XCHG32 for riscv qspinlock
- Disable xhg16/cmxchg16 and any sub-word atomic primitive in riscv
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic warning on PCI_IOBASE
On Tue, Apr 13, 2021 at 9:40 PM Arnd Bergmann wrote: > > On Tue, Apr 13, 2021 at 3:06 PM David Laight wrote: > > > > From: Arnd Bergmann > > > Sent: 13 April 2021 13:58 > > ... > > > The remaining ones (csky, m68k, sparc32) need to be inspected > > > manually to see if they currently support PCI I/O space but in > > > fact use address zero as the base (with large resources) or they > > > should also turn the operations into a NOP. > > > > I'd expect sparc32 to use an ASI to access PCI IO space. > > I can't quite remember whether IO space was supported at all. > > I see this bit in arch/sparc/kernel/leon_pci.c > > * PCI Memory and Prefetchable Memory is direct-mapped. However I/O Space is > * accessed through a Window which is translated to low 64KB in PCI space, the > * first 4KB is not used so 60KB is available. > ... > pci_add_resource_offset(, >io_space, > info->io_space.start - 0x1000); > > which means that there is I/O space, which gets accessed through whichever > method readb() uses. Having the offset equal to the resource means that > the '(void *)0' start is correct. > > As this leaves only two others, I checked those as well: > > csky does not actually have a PCI host bridge driver at the moment, so > we don't care about breaking port access on it it, and I would suggest > leaving I/O port access disabled. (Added Guo Ren to Cc for confirmation). Yes, we haven't reserved the PCI_IO region in the VM layout. > > m68k only supports PCI on coldfire M54xx, and this variant does set > a PCI_IOBASE after all. The normal MMU based m68k have no PCI > and do define their out inb/outb/..., so nothing changes for them. > > To summarize: only sparc32 needs to set PCI_IOBASE to zero, everyone > else should just WARN_ONCE() or return 0xff/0x/0x. > > Arnd -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
On Tue, Apr 13, 2021 at 6:25 PM Christoph Müllner
wrote:
>
> On Tue, Apr 13, 2021 at 11:37 AM Peter Zijlstra wrote:
> >
> > On Tue, Apr 13, 2021 at 11:22:40AM +0200, Christoph Müllner wrote:
> >
> > > > For ticket locks you really only needs atomic_fetch_add() and
> > > > smp_store_release() and an architectural guarantees that the
> > > > atomic_fetch_add() has fwd progress under contention and that a sub-word
> > > > store (through smp_store_release()) will fail the SC.
> > > >
> > > > Then you can do something like:
> > > >
> > > > void lock(atomic_t *lock)
> > > > {
> > > > u32 val = atomic_fetch_add(1<<16, lock); /* SC, gives us RCsc */
> > > > u16 ticket = val >> 16;
> > > >
> > > > for (;;) {
> > > > if (ticket == (u16)val)
> > > > break;
> > > > cpu_relax();
> > > > val = atomic_read_acquire(lock);
> > > > }
> > > > }
> > > >
> > > > void unlock(atomic_t *lock)
> > > > {
> > > > u16 *ptr = (u16 *)lock + (!!__BIG_ENDIAN__);
> > > > u32 val = atomic_read(lock);
> > > >
> > > > smp_store_release(ptr, (u16)val + 1);
> > > > }
> > > >
> > > > That's _almost_ as simple as a test-and-set :-) It isn't quite optimal
> > > > on x86 for not being allowed to use a memop on unlock, since its being
> > > > forced into a load-store because of all the volatile, but whatever.
> > >
> > > What about trylock()?
> > > I.e. one could implement trylock() without a loop, by letting
> > > trylock() fail if the SC fails.
> > > That looks safe on first view, but nobody does this right now.
> >
> > Generic code has to use cmpxchg(), and then you get something like this:
> >
> > bool trylock(atomic_t *lock)
> > {
> > u32 old = atomic_read(lock);
> >
> > if ((old >> 16) != (old & 0x))
> > return false;
> >
> > return atomic_try_cmpxchg(lock, , old + (1<<16)); /* SC, for
> > RCsc */
> > }
>
> This approach requires two loads (atomic_read() and cmpxchg()), which
> is not required.
> Detecting this pattern and optimizing it in a compiler is quite unlikely.
>
> A bit less generic solution would be to wrap the LL/SC (would be
> mandatory in this case)
> instructions and do something like this:
>
> uint32_t __smp_load_acquire_reserved(void*);
> int __smp_store_release_conditional(void*, uint32_t);
>
> typedef union {
> uint32_t v32;
> struct {
> uint16_t owner;
> uint16_t next;
> };
> } arch_spinlock_t;
>
> int trylock(arch_spinlock_t *lock)
> {
> arch_spinlock_t l;
> int success;
> do {
> l.v32 = __smp_load_acquire_reserved(lock);
> if (l.owner != l.next)
> return 0;
> l.next++;
> success = __smp_store_release_conditional(lock, l.v32);
It's a new semantics v.s cmpxchg, and cmpxchg is come from CAS
instruction to solve some complex scenario.
The primitive of cmpxchg has been widely used in Linux, so I don't
think introducing a new semantics (reserved_conditional) is a good
idea.
Also, from this point: It seems CAS instruction is more suitable for
software compatibility. Although riscv privilege spec chose the LR/SC
and list some bad sides of CAS.
> } while (!success);
> return success;
> }
>
> But here we can't tell the compiler to optimize the code between LL and SC...
>
> >
> > That will try and do the full LL/SC loop, because it wants to complete
> > the cmpxchg, but in generic code we have no other option.
> >
> > (Is this what C11's weak cmpxchg is for?)
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: fix syscache.c fallthrough warning
Acked-by: Guo Ren
It's a fallthrough is for BCACHE, but affects ICACHE with more
expensive. I'll fix up it later.
}
On Mon, Apr 12, 2021 at 12:41 AM Randy Dunlap wrote:
>
> This case of the switch statement falls through to the following case.
> This appears to be on purpose, so declare it as OK.
>
> ../arch/csky/mm/syscache.c: In function '__do_sys_cacheflush':
> ../arch/csky/mm/syscache.c:17:3: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
>17 | flush_icache_mm_range(current->mm,
> | ^~
>18 | (unsigned long)addr,
> |
>19 | (unsigned long)addr + bytes);
> |
> ../arch/csky/mm/syscache.c:20:2: note: here
>20 | case DCACHE:
> | ^~~~
>
> Fixes: 997153b9a75c ("csky: Add flush_icache_mm to defer flush icache all")
> Signed-off-by: Randy Dunlap
> Cc: Guo Ren
> Cc: linux-c...@vger.kernel.org
> Cc: Arnd Bergmann
> ---
> @Guo, should this be a "break" instead of fallthrough?
>
> arch/csky/mm/syscache.c |1 +
> 1 file changed, 1 insertion(+)
>
> --- linux-next-20210409.orig/arch/csky/mm/syscache.c
> +++ linux-next-20210409/arch/csky/mm/syscache.c
> @@ -17,6 +17,7 @@ SYSCALL_DEFINE3(cacheflush,
> flush_icache_mm_range(current->mm,
> (unsigned long)addr,
> (unsigned long)addr + bytes);
> + fallthrough;
> case DCACHE:
> dcache_wb_range((unsigned long)addr,
> (unsigned long)addr + bytes);
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v6 3/9] riscv: locks: Introduce ticket-based spinlock implementation
On Mon, Apr 12, 2021 at 12:02 AM Guo Ren wrote:
>
> On Wed, Mar 31, 2021 at 10:32 PM wrote:
> >
> > From: Guo Ren
> >
> > This patch introduces a ticket lock implementation for riscv, along the
> > same lines as the implementation for arch/arm & arch/csky.
> >
> > We still use qspinlock as default.
> >
> > Signed-off-by: Guo Ren
> > Cc: Peter Zijlstra
> > Cc: Anup Patel
> > Cc: Arnd Bergmann
> > ---
> > arch/riscv/Kconfig | 7 ++-
> > arch/riscv/include/asm/spinlock.h | 84 +
> > arch/riscv/include/asm/spinlock_types.h | 17 +
> > 3 files changed, 107 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 67cc65ba1ea1..34d0276f01d5 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -34,7 +34,7 @@ config RISCV
> > select ARCH_WANT_FRAME_POINTERS
> > select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
> > select ARCH_USE_QUEUED_RWLOCKS
> > - select ARCH_USE_QUEUED_SPINLOCKS
> > + select ARCH_USE_QUEUED_SPINLOCKSif !RISCV_TICKET_LOCK
> > select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> > select CLONE_BACKWARDS
> > select CLINT_TIMER if !MMU
> > @@ -344,6 +344,11 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
> > def_bool y
> > depends on NUMA
> >
> > +config RISCV_TICKET_LOCK
> > + bool "Ticket-based spin-locking"
> > + help
> > + Say Y here to use ticket-based spin-locking.
> > +
> > config RISCV_ISA_C
> > bool "Emit compressed instructions when building Linux"
> > default y
> > diff --git a/arch/riscv/include/asm/spinlock.h
> > b/arch/riscv/include/asm/spinlock.h
> > index a557de67a425..90b7eaa950cf 100644
> > --- a/arch/riscv/include/asm/spinlock.h
> > +++ b/arch/riscv/include/asm/spinlock.h
> > @@ -7,7 +7,91 @@
> > #ifndef _ASM_RISCV_SPINLOCK_H
> > #define _ASM_RISCV_SPINLOCK_H
> >
> > +#ifdef CONFIG_RISCV_TICKET_LOCK
> > +#ifdef CONFIG_32BIT
> > +#define __ASM_SLLIW "slli\t"
> > +#define __ASM_SRLIW "srli\t"
> > +#else
> > +#define __ASM_SLLIW "slliw\t"
> > +#define __ASM_SRLIW "srliw\t"
> > +#endif
> > +
> > +/*
> > + * Ticket-based spin-locking.
> > + */
> > +static inline void arch_spin_lock(arch_spinlock_t *lock)
> > +{
> > + arch_spinlock_t lockval;
> > + u32 tmp;
> > +
> > + asm volatile (
> > + "1: lr.w%0, %2 \n"
> > + " mv %1, %0 \n"
> > + " addw%0, %0, %3 \n"
> > + " sc.w%0, %0, %2 \n"
> > + " bnez%0, 1b \n"
> > + : "=" (tmp), "=" (lockval), "+A" (lock->lock)
> > + : "r" (1 << TICKET_NEXT)
> > + : "memory");
> > +
> > + smp_cond_load_acquire(>tickets.owner,
> > + VAL == lockval.tickets.next);
> It's wrong, blew is fixup:
>
> diff --git a/arch/csky/include/asm/spinlock.h
> b/arch/csky/include/asm/spinlock.h
> index fe98ad8ece51..2be627ceb9df 100644
> --- a/arch/csky/include/asm/spinlock.h
> +++ b/arch/csky/include/asm/spinlock.h
> @@ -27,7 +27,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> : "r"(p), "r"(ticket_next)
> : "cc");
>
> - smp_cond_load_acquire(>tickets.owner,
> + if (lockval.owner != lockval.tickets.next)
> + smp_cond_load_acquire(>tickets.owner,
> VAL == lockval.tickets.next);
eh... plus __smp_acquire_fence:
if (lockval.owner != lockval.tickets.next)
smp_cond_load_acquire(>tickets.owner,
VAL == lockval.tickets.next);
else
__smp_acquire_fence();
> > +}
> > +
> > +static inline int arch_spin_trylock(arch_spinlock_t *lock)
> > +{
> > + u32 tmp, contended, res;
> > +
> > + do {
> > + asm volatile (
> > + " lr.w%0, %3 \n"
> > + __ASM_SRLIW"%1, %0, %5 \n"
> > + __ASM_S
Re: [PATCH v6 3/9] riscv: locks: Introduce ticket-based spinlock implementation
On Wed, Mar 31, 2021 at 10:32 PM wrote:
>
> From: Guo Ren
>
> This patch introduces a ticket lock implementation for riscv, along the
> same lines as the implementation for arch/arm & arch/csky.
>
> We still use qspinlock as default.
>
> Signed-off-by: Guo Ren
> Cc: Peter Zijlstra
> Cc: Anup Patel
> Cc: Arnd Bergmann
> ---
> arch/riscv/Kconfig | 7 ++-
> arch/riscv/include/asm/spinlock.h | 84 +
> arch/riscv/include/asm/spinlock_types.h | 17 +
> 3 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 67cc65ba1ea1..34d0276f01d5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -34,7 +34,7 @@ config RISCV
> select ARCH_WANT_FRAME_POINTERS
> select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
> select ARCH_USE_QUEUED_RWLOCKS
> - select ARCH_USE_QUEUED_SPINLOCKS
> + select ARCH_USE_QUEUED_SPINLOCKSif !RISCV_TICKET_LOCK
> select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> select CLONE_BACKWARDS
> select CLINT_TIMER if !MMU
> @@ -344,6 +344,11 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
> def_bool y
> depends on NUMA
>
> +config RISCV_TICKET_LOCK
> + bool "Ticket-based spin-locking"
> + help
> + Say Y here to use ticket-based spin-locking.
> +
> config RISCV_ISA_C
> bool "Emit compressed instructions when building Linux"
> default y
> diff --git a/arch/riscv/include/asm/spinlock.h
> b/arch/riscv/include/asm/spinlock.h
> index a557de67a425..90b7eaa950cf 100644
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -7,7 +7,91 @@
> #ifndef _ASM_RISCV_SPINLOCK_H
> #define _ASM_RISCV_SPINLOCK_H
>
> +#ifdef CONFIG_RISCV_TICKET_LOCK
> +#ifdef CONFIG_32BIT
> +#define __ASM_SLLIW "slli\t"
> +#define __ASM_SRLIW "srli\t"
> +#else
> +#define __ASM_SLLIW "slliw\t"
> +#define __ASM_SRLIW "srliw\t"
> +#endif
> +
> +/*
> + * Ticket-based spin-locking.
> + */
> +static inline void arch_spin_lock(arch_spinlock_t *lock)
> +{
> + arch_spinlock_t lockval;
> + u32 tmp;
> +
> + asm volatile (
> + "1: lr.w%0, %2 \n"
> + " mv %1, %0 \n"
> + " addw%0, %0, %3 \n"
> + " sc.w%0, %0, %2 \n"
> + " bnez%0, 1b \n"
> + : "=" (tmp), "=" (lockval), "+A" (lock->lock)
> + : "r" (1 << TICKET_NEXT)
> + : "memory");
> +
> + smp_cond_load_acquire(>tickets.owner,
> + VAL == lockval.tickets.next);
It's wrong, blew is fixup:
diff --git a/arch/csky/include/asm/spinlock.h b/arch/csky/include/asm/spinlock.h
index fe98ad8ece51..2be627ceb9df 100644
--- a/arch/csky/include/asm/spinlock.h
+++ b/arch/csky/include/asm/spinlock.h
@@ -27,7 +27,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
: "r"(p), "r"(ticket_next)
: "cc");
- smp_cond_load_acquire(>tickets.owner,
+ if (lockval.owner != lockval.tickets.next)
+ smp_cond_load_acquire(>tickets.owner,
VAL == lockval.tickets.next);
> +}
> +
> +static inline int arch_spin_trylock(arch_spinlock_t *lock)
> +{
> + u32 tmp, contended, res;
> +
> + do {
> + asm volatile (
> + " lr.w%0, %3 \n"
> + __ASM_SRLIW"%1, %0, %5 \n"
> + __ASM_SLLIW"%2, %0, %5 \n"
> + " or %1, %2, %1 \n"
> + " li %2, 0 \n"
> + " sub %1, %1, %0 \n"
> + " bnez%1, 1f \n"
> + " addw%0, %0, %4 \n"
> + " sc.w%2, %0, %3 \n"
> + "1: \n"
> + : "=" (tmp), "=" (contended), "=" (res),
> + "+A" (lock->lock)
> + : "r" (1 << TICKET_NEXT), "I" (TICKET_NEXT)
> + : "memory");
> + } while (res);
> +
> + if (!contended)
> +
Re: [PATCH v6 4/9] csky: locks: Optimize coding convention
On Wed, Mar 31, 2021 at 10:32 PM wrote:
>
> From: Guo Ren
>
> - Using smp_cond_load_acquire in arch_spin_lock by Peter's
>advice.
> - Using __smp_acquire_fence in arch_spin_trylock
> - Using smp_store_release in arch_spin_unlock
>
> All above are just coding conventions and won't affect the
> function.
>
> TODO in smp_cond_load_acquire for architecture:
> - current csky only has:
>lr.w val,
>sc.w . val2
>(Any other stores to p0 will let sc.w failed)
>
> - But smp_cond_load_acquire need:
>lr.w val,
>wfe
>(Any stores to p0 will send the event to let wfe retired)
>
> Signed-off-by: Guo Ren
> Link:
> https://lore.kernel.org/linux-riscv/caahsdy1jhlufwu7rucaq+ruwrbks2ksdva7eprt8--4zfof...@mail.gmail.com/T/#m13adac285b7f51f4f879a5d6b65753ecb1a7524e
> Cc: Peter Zijlstra
> Cc: Arnd Bergmann
> ---
> arch/csky/include/asm/spinlock.h | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/csky/include/asm/spinlock.h
> b/arch/csky/include/asm/spinlock.h
> index 69f5aa249c5f..69677167977a 100644
> --- a/arch/csky/include/asm/spinlock.h
> +++ b/arch/csky/include/asm/spinlock.h
> @@ -26,10 +26,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> : "r"(p), "r"(ticket_next)
> : "cc");
>
> - while (lockval.tickets.next != lockval.tickets.owner)
> - lockval.tickets.owner = READ_ONCE(lock->tickets.owner);
> -
> - smp_mb();
> + smp_cond_load_acquire(>tickets.owner,
> + VAL == lockval.tickets.next);
It's wrong, we should determine lockval before next read.
Fixup:
diff --git a/arch/csky/include/asm/spinlock.h b/arch/csky/include/asm/spinlock.h
index fe98ad8ece51..2be627ceb9df 100644
--- a/arch/csky/include/asm/spinlock.h
+++ b/arch/csky/include/asm/spinlock.h
@@ -27,7 +27,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
: "r"(p), "r"(ticket_next)
: "cc");
- smp_cond_load_acquire(>tickets.owner,
+ if (lockval.owner != lockval.tickets.next)
+ smp_cond_load_acquire(>tickets.owner,
VAL == lockval.tickets.next);
> }
>
> static inline int arch_spin_trylock(arch_spinlock_t *lock)
> @@ -55,15 +53,14 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
> } while (!res);
>
> if (!contended)
> - smp_mb();
> + __smp_acquire_fence();
>
> return !contended;
> }
>
> static inline void arch_spin_unlock(arch_spinlock_t *lock)
> {
> - smp_mb();
> - WRITE_ONCE(lock->tickets.owner, lock->tickets.owner + 1);
> + smp_store_release(>tickets.owner, lock->tickets.owner + 1);
> }
>
> static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
> --
> 2.17.1
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Mar 31, 2021 at 3:23 PM Arnd Bergmann wrote: > > On Wed, Mar 31, 2021 at 12:35 AM Stafford Horne wrote: > > > > I just want to chime in here, there may be a better spot in the thread to > > mention this but, for OpenRISC I did implement some generic 8/16-bit xchg > > code > > which I have on my todo list somwhere to replace the other generic > > implementations like that in mips. > > > > arch/openrisc/include/asm/cmpxchg.h > > > > The idea would be that architectures just implement these methods: > > > > long cmpxchg_u32(*ptr,old,new) > > long xchg_u32(*ptr,val) > > > > Then the rest of the generic header would implement cmpxchg. > > I like the idea of generalizing it a little further. I'd suggest staying a > little closer to the existing naming here though, as we already have > cmpxchg() for the type-agnostic version, and cmpxchg64() for the > fixed-length 64-bit version. > > I think a nice interface between architecture-specific and architecture > independent code would be to have architectures provide > arch_cmpxchg32()/arch_xchg32() as the most basic version, as well > as arch_cmpxchg8()/arch_cmpxchg16()/arch_xchg8()/arch_xchg16() > if they have instructions for those. > > The common code can then build cmpxchg16()/xchg16() on top of > either the 16-bit or the 32-bit primitives, and build the cmpxchg()/xchg() > wrapper around those (or alternatively we can decide to have them > only deal with fixed-32-bit and long/pointer sized atomics). I think these emulation codes are suitable for some architectures but not riscv. We shouldn't export xchg16/cmpxchg16(emulated by lr.w/sc.w) in riscv, We should forbid these sub-word atomic primitive and lets the programmers consider their atomic design. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: remove unused including
Acked by On Tue, Apr 6, 2021 at 9:48 AM Yang Li wrote: > > Fix the following versioncheck warnings: > ./arch/csky/include/asm/io.h: 8 linux/version.h not needed. > ./arch/csky/include/asm/uaccess.h: 14 linux/version.h not needed. > ./arch/csky/kernel/process.c: 5 linux/version.h not needed. > ./arch/csky/mm/dma-mapping.c: 12 linux/version.h not needed. > > Reported-by: Abaci Robot > Signed-off-by: Yang Li > --- > arch/csky/include/asm/io.h | 1 - > arch/csky/include/asm/uaccess.h | 1 - > arch/csky/kernel/process.c | 1 - > arch/csky/mm/dma-mapping.c | 1 - > 4 files changed, 4 deletions(-) > > diff --git a/arch/csky/include/asm/io.h b/arch/csky/include/asm/io.h > index f826540..ed53f0b 100644 > --- a/arch/csky/include/asm/io.h > +++ b/arch/csky/include/asm/io.h > @@ -5,7 +5,6 @@ > > #include > #include > -#include > > /* > * I/O memory access primitives. Reads are ordered relative to any > diff --git a/arch/csky/include/asm/uaccess.h b/arch/csky/include/asm/uaccess.h > index 3dec272..841e421 100644 > --- a/arch/csky/include/asm/uaccess.h > +++ b/arch/csky/include/asm/uaccess.h > @@ -11,7 +11,6 @@ > #include > #include > #include > -#include > #include > > static inline int access_ok(const void *addr, unsigned long size) > diff --git a/arch/csky/kernel/process.c b/arch/csky/kernel/process.c > index 3d0ca22..5de0470 100644 > --- a/arch/csky/kernel/process.c > +++ b/arch/csky/kernel/process.c > @@ -2,7 +2,6 @@ > // Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd. > > #include > -#include > #include > #include > #include > diff --git a/arch/csky/mm/dma-mapping.c b/arch/csky/mm/dma-mapping.c > index c3a775a..8244702 100644 > --- a/arch/csky/mm/dma-mapping.c > +++ b/arch/csky/mm/dma-mapping.c > @@ -9,7 +9,6 @@ > #include > #include > #include > -#include > #include > > static inline void cache_op(phys_addr_t paddr, size_t size, > -- > 1.8.3.1 > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 10:09 PM Waiman Long wrote:
>
> On 3/29/21 11:13 PM, Guo Ren wrote:
> > On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote:
> >> On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote:
> >>> u32 a = 0x55aa66bb;
> >>> u16 *ptr =
> >>>
> >>> CPU0 CPU1
> >>> = =
> >>> xchg16(ptr, new) while(1)
> >>> WRITE_ONCE(*(ptr + 1), x);
> >>>
> >>> When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock.
> >> Then I think your LL/SC is broken.
> >>
> >> That also means you really don't want to build super complex locking
> >> primitives on top, because that live-lock will percolate through.
> > Do you mean the below implementation has live-lock risk?
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + u32 old, new, val = atomic_read(>val);
> > +
> > + for (;;) {
> > + new = (val & _Q_LOCKED_PENDING_MASK) | tail;
> > + old = atomic_cmpxchg(>val, val, new);
> > + if (old == val)
> > + break;
> > +
> > + val = old;
> > + }
> > + return old;
> > +}
> If there is a continuous stream of incoming spinlock takers, it is
> possible that some cpus may have to wait a long time to set the tail
> right. However, this should only happen on artificial workload. I doubt
> it will happen with real workload or with limit number of cpus.
Yes, I think is ok for LR/SC in riscv, becasue
CPU0 LR
CPU1 LR
CPU0 SC //success
CPU1 SC //fail
or
CPU0 LR
CPU1 LR
CPU1 SC //success
CPU0 SC //fail
So always one store condition would success. I think it's OK.
> >
> >> Step 1 would be to get your architecute fixed such that it can provide
> >> fwd progress guarantees for LL/SC. Otherwise there's absolutely no point
> >> in building complex systems with it.
> > Quote Waiman's comment [1] on xchg16 optimization:
> >
> > "This optimization is needed to make the qspinlock achieve performance
> > parity with ticket spinlock at light load."
> >
> > [1]
> > https://lore.kernel.org/kvm/1429901803-29771-6-git-send-email-waiman.l...@hp.com/
> >
> > So for a non-xhg16 machine:
> > - ticket-lock for small numbers of CPUs
> > - qspinlock for large numbers of CPUs
> >
> > Okay, I'll put all of them into the next patch :P
> >
> It is true that qspinlock may not offer much advantage when the number
> of cpus is small. It shines for systems with many cpus. You may use
> NR_CPUS to determine if the default should be ticket or qspinlock with
> user override. To determine the right NR_CPUS threshold, you may need to
> run on real SMP RISCV systems to find out.
I Agree
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Wed, Mar 31, 2021 at 12:08 AM Peter Zijlstra wrote:
>
> On Tue, Mar 30, 2021 at 11:13:55AM +0800, Guo Ren wrote:
> > On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote:
> > >
> > > On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote:
> > > > u32 a = 0x55aa66bb;
> > > > u16 *ptr =
> > > >
> > > > CPU0 CPU1
> > > > = =
> > > > xchg16(ptr, new) while(1)
> > > > WRITE_ONCE(*(ptr + 1), x);
> > > >
> > > > When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock.
> > >
> > > Then I think your LL/SC is broken.
> > >
> > > That also means you really don't want to build super complex locking
> > > primitives on top, because that live-lock will percolate through.
>
> > Do you mean the below implementation has live-lock risk?
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + u32 old, new, val = atomic_read(>val);
> > +
> > + for (;;) {
> > + new = (val & _Q_LOCKED_PENDING_MASK) | tail;
> > + old = atomic_cmpxchg(>val, val, new);
> > + if (old == val)
> > + break;
> > +
> > + val = old;
> > + }
> > + return old;
> > +}
>
> That entirely depends on the architecture (and cmpxchg() impementation).
>
> There are a number of cases:
>
> * architecture has cmpxchg() instruction (x86, s390, sparc, etc.).
>
> - architecture provides fwd progress (x86)
> - architecture requires backoff for progress (sparc)
>
> * architecture does not have cmpxchg, and implements it using LL/SC.
>
> and here things get *really* interesting, because while an
> architecture can have LL/SC fwd progress, that does not translate into
> cmpxchg() also having the same guarantees and all bets are off.
Seems riscv spec didn't mandatory LR/SC fwd guarantee, ref:
In riscv-spec 8.3 Eventual Success of Store-Conditional Instructions
"As a consequence of the eventuality guarantee, if some harts in an
execution environment are executing constrained LR/SC loops, and no
other harts or devices in the execution environment execute an
unconditional store or AMO to that reservation set, then at least one
hart will eventually exit its constrained LR/SC loop. *** By contrast,
if other harts or devices continue to write to that reservation set,
it ***is not guaranteed*** that any hart will exit its LR/SC loop.***
"
>
> The real bummer is that C can do cmpxchg(), but there is no way it can
> do LL/SC. And even if we'd teach C how to do LL/SC, it couldn't be
> generic because architectures lacking it can't emulate it using
> cmpxchg() (there's a fun class of bugs there).
>
> So while the above code might be the best we can do in generic code,
> it's really up to the architecture to make it work.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Hi Paul, Thx for the explanation, here is my comment. On Wed, Mar 31, 2021 at 1:33 PM Paul Campbell wrote: > > On Wednesday, 31 March 2021 5:18:56 PM NZDT Guo Ren wrote: > > > > [1] > > > > https://github.com/c-sky/csky-linux/commit/e837aad23148542771794d8a2fcc > > > > 52afd0fcbf88> > > > > > > It also seems that the current "amoswap" based implementation > > > > > would be reliable independent of RsrvEventual/RsrvNonEventual. > > > > > > > > Yes, the hardware implementation of AMO could be different from LR/SC. > > > > AMO could use ACE snoop holding to lock the bus in hw coherency > > > > design, but LR/SC uses an exclusive monitor without locking the bus. > > > > > > > > RISC-V hasn't CAS instructions, and it uses LR/SC for cmpxchg. I don't > > > > think LR/SC would be slower than CAS, and CAS is just good for code > > > > size. > > > > > > What I meant here is that the current spinlock uses a simple amoswap, > > > which presumably does not suffer from the lack of forward process you > > > described. > > > > Does that mean we should prevent using LR/SC (if RsrvNonEventual)? > > Let me provide another data-point, I'm working on a high-end highly > speculative implementation with many concurrent instructions in flight - from > my point of view both sorts of AMO (LR/SC and swap/add/etc) require me to > grab a cache line in an exclusive modifiable state (so no difference there). > > More importantly both sorts of AMO instructions (unlike most loads and > stores) can't be speculated (not even LR because it changes hidden state, I > found this out the hard way bringing up the kernel). > > This means that both LR AND SC individually can't be executed until all > speculation is resolved (that means that they happen really late in the > execute path and block the resolution of the speculation of subsequent > instructions) - equally a single amoswap/add/etc instruction can't happen > until late in the execute path - so both require the same cache line state, > but one of these sorts of events is better than two of them. > > Which in short means that amoswap/add/etc is better for small things - small > buzzy lock loops, while LR/SC is better for more complex things with actual > processing between the LR and SC. Seems your machine using the same way to implement LR/SC and AMO, but some machines would differ them. For AMO, I think it's would be like what you've described: - AMO would be separated into three parts: load & lock, ALU operation, store & unlock - load & lock, eg: we could using ACE protocol -SNOOP channel to holding the bus - Doing atomic AMO - store & unlock: Write the result back and releasing the ACE protocol -SNOOP channel I think the above is what you describe as how to "grab a cache line in an exclusive modifiable state". But for LR/SC, it's different. Because we have separated AMO into real three parts of instruction: - LR - Operation instructions - SC If we let LR holding ACE protocol -SNOOP channel and let SC release channel, that would break the ISA design (we couldn't let an instruction holding the snoop bus and made other harts hang up.) So LR/SC would use address monitors for every hart, to detect the target address has been written or not. That means LR/SC won't be implemented fwd progress guarantees. If you care about fwd progress guarantees, I think ISA should choose cmpxchg (eg: cas) instead of LR/SC. > > > > Another issue here is to consider is what happens when you hit one of these > tight spinlocks when the branch target cache is empty and they fail (ie loop > back and try again) - the default branch prediction, and resulting > speculation, is (very) likely to be looping back, while hopefully most locks > are not contended when you hit them and that speculation would be wrong - a > spinlock like this may not be so good: > > li a0, 1 > loop: > amoswap a1, a0, (a2) > beqza1, loop > . subsequent code > > In my world with no BTC info the pipe fills with dozens of amoswaps, rather > than the 'subsequent code'. While (in my world) code like this: > > li a0, 1 > loop: > amoswap a1, a0, (a2) > bneza1, 1f > subsequent code > > 1: j loop > > would actually be better (in my world unconditional jump instructions are > folded early and never see execution so they're sort of free, though they mess > with the issue/decode rate). Smart compilers could move the "j loop" out of > the way, while the double branch on failure is not a big deal since either the > lock is still h
Re: [PATCH v6 3/9] riscv: locks: Introduce ticket-based spinlock implementation
On Wed, Mar 31, 2021 at 10:32 PM wrote:
>
> From: Guo Ren
>
> This patch introduces a ticket lock implementation for riscv, along the
> same lines as the implementation for arch/arm & arch/csky.
>
> We still use qspinlock as default.
>
> Signed-off-by: Guo Ren
> Cc: Peter Zijlstra
> Cc: Anup Patel
> Cc: Arnd Bergmann
> ---
> arch/riscv/Kconfig | 7 ++-
> arch/riscv/include/asm/spinlock.h | 84 +
> arch/riscv/include/asm/spinlock_types.h | 17 +
> 3 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 67cc65ba1ea1..34d0276f01d5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -34,7 +34,7 @@ config RISCV
> select ARCH_WANT_FRAME_POINTERS
> select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
> select ARCH_USE_QUEUED_RWLOCKS
> - select ARCH_USE_QUEUED_SPINLOCKS
> + select ARCH_USE_QUEUED_SPINLOCKSif !RISCV_TICKET_LOCK
> select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> select CLONE_BACKWARDS
> select CLINT_TIMER if !MMU
> @@ -344,6 +344,11 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
> def_bool y
> depends on NUMA
>
> +config RISCV_TICKET_LOCK
> + bool "Ticket-based spin-locking"
> + help
> + Say Y here to use ticket-based spin-locking.
> +
> config RISCV_ISA_C
> bool "Emit compressed instructions when building Linux"
> default y
> diff --git a/arch/riscv/include/asm/spinlock.h
> b/arch/riscv/include/asm/spinlock.h
> index a557de67a425..90b7eaa950cf 100644
> --- a/arch/riscv/include/asm/spinlock.h
> +++ b/arch/riscv/include/asm/spinlock.h
> @@ -7,7 +7,91 @@
> #ifndef _ASM_RISCV_SPINLOCK_H
> #define _ASM_RISCV_SPINLOCK_H
>
> +#ifdef CONFIG_RISCV_TICKET_LOCK
> +#ifdef CONFIG_32BIT
> +#define __ASM_SLLIW "slli\t"
> +#define __ASM_SRLIW "srli\t"
> +#else
> +#define __ASM_SLLIW "slliw\t"
> +#define __ASM_SRLIW "srliw\t"
> +#endif
> +
> +/*
> + * Ticket-based spin-locking.
> + */
> +static inline void arch_spin_lock(arch_spinlock_t *lock)
> +{
> + arch_spinlock_t lockval;
> + u32 tmp;
> +
> + asm volatile (
> + "1: lr.w%0, %2 \n"
> + " mv %1, %0 \n"
> + " addw%0, %0, %3 \n"
> + " sc.w%0, %0, %2 \n"
> + " bnez%0, 1b \n"
> + : "=" (tmp), "=" (lockval), "+A" (lock->lock)
> + : "r" (1 << TICKET_NEXT)
> + : "memory");
It's could be optimized by amoadd.w with Anup advice, and I'll update
it in the next patchset version:
diff --git a/arch/riscv/include/asm/spinlock.h
b/arch/riscv/include/asm/spinlock.h
index 90b7eaa950cf..435286ad342b 100644
--- a/arch/riscv/include/asm/spinlock.h
+++ b/arch/riscv/include/asm/spinlock.h
@@ -22,15 +22,10 @@
static inline void arch_spin_lock(arch_spinlock_t *lock)
{
arch_spinlock_t lockval;
- u32 tmp;
asm volatile (
- "1: lr.w%0, %2 \n"
- " mv %1, %0 \n"
- " addw%0, %0, %3 \n"
- " sc.w%0, %0, %2 \n"
- " bnez%0, 1b \n"
- : "=" (tmp), "=" (lockval), "+A" (lock->lock)
+ " amoadd.w%0, %2, %1 \n"
+ : "=" (lockval), "+A" (lock->lock)
: "r" (1 << TICKET_NEXT)
: "memory");
> +
> + smp_cond_load_acquire(>tickets.owner,
> + VAL == lockval.tickets.next);
> +}
> +
> +static inline int arch_spin_trylock(arch_spinlock_t *lock)
> +{
> + u32 tmp, contended, res;
> +
> + do {
> + asm volatile (
> + " lr.w%0, %3 \n"
> + __ASM_SRLIW"%1, %0, %5 \n"
> + __ASM_SLLIW"%2, %0, %5 \n"
> + " or %1, %2, %1 \n"
> + " li %2, 0 \n"
> + " sub %1, %1, %0 \n"
> + " bnez%1, 1f \n"
> + " addw%0, %0, %4 \n"
> + &quo
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote: > > On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote: > > u32 a = 0x55aa66bb; > > u16 *ptr = > > > > CPU0 CPU1 > > = = > > xchg16(ptr, new) while(1) > > WRITE_ONCE(*(ptr + 1), x); > > > > When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock. > > Then I think your LL/SC is broken. No, it's not broken LR.W/SC.W. Quote <8.3 Eventual Success of Store-Conditional Instructions>: "As a consequence of the eventuality guarantee, if some harts in an execution environment are executing constrained LR/SC loops, and no other harts or devices in the execution environment execute an unconditional store or AMO to that reservation set, then at least one hart will eventually exit its constrained LR/SC loop. By contrast, if other harts or devices continue to write to that reservation set, it is not guaranteed that any hart will exit its LR/SC loop." So I think it's a feature of LR/SC. How does the above code (also use ll.w/sc.w to implement xchg16) running on arm64? 1: ldxr eor cbnz ... 2f stxr cbnz ... 1b // I think it would deadlock for arm64. "LL/SC fwd progress" which you have mentioned could guarantee stxr success? How hardware could do that? > > That also means you really don't want to build super complex locking > primitives on top, because that live-lock will percolate through. > > Step 1 would be to get your architecute fixed such that it can provide > fwd progress guarantees for LL/SC. Otherwise there's absolutely no point > in building complex systems with it. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Hi Stafford, How do think add ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in openrisc? https://lore.kernel.org/linux-riscv/1617201040-83905-7-git-send-email-guo...@kernel.org/T/#u On Wed, Mar 31, 2021 at 8:31 PM Stafford Horne wrote: > > On Wed, Mar 31, 2021 at 09:23:27AM +0200, Arnd Bergmann wrote: > > On Wed, Mar 31, 2021 at 12:35 AM Stafford Horne wrote: > > > > > > I just want to chime in here, there may be a better spot in the thread to > > > mention this but, for OpenRISC I did implement some generic 8/16-bit xchg > > > code > > > which I have on my todo list somwhere to replace the other generic > > > implementations like that in mips. > > > > > > arch/openrisc/include/asm/cmpxchg.h > > > > > > The idea would be that architectures just implement these methods: > > > > > > long cmpxchg_u32(*ptr,old,new) > > > long xchg_u32(*ptr,val) > > > > > > Then the rest of the generic header would implement cmpxchg. > > > > I like the idea of generalizing it a little further. I'd suggest staying a > > little closer to the existing naming here though, as we already have > > cmpxchg() for the type-agnostic version, and cmpxchg64() for the > > fixed-length 64-bit version. > > OK. > > > I think a nice interface between architecture-specific and architecture > > independent code would be to have architectures provide > > arch_cmpxchg32()/arch_xchg32() as the most basic version, as well > > as arch_cmpxchg8()/arch_cmpxchg16()/arch_xchg8()/arch_xchg16() > > if they have instructions for those. > > Thanks for the name suggestions, it makes it easier for me. > > > The common code can then build cmpxchg16()/xchg16() on top of > > either the 16-bit or the 32-bit primitives, and build the cmpxchg()/xchg() > > wrapper around those (or alternatively we can decide to have them > > only deal with fixed-32-bit and long/pointer sized atomics). > > Yeah, that was the idea. > > -Stafford -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 10:09 PM Waiman Long wrote:
>
> On 3/29/21 11:13 PM, Guo Ren wrote:
> > On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote:
> >> On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote:
> >>> u32 a = 0x55aa66bb;
> >>> u16 *ptr =
> >>>
> >>> CPU0 CPU1
> >>> = =
> >>> xchg16(ptr, new) while(1)
> >>> WRITE_ONCE(*(ptr + 1), x);
> >>>
> >>> When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock.
> >> Then I think your LL/SC is broken.
> >>
> >> That also means you really don't want to build super complex locking
> >> primitives on top, because that live-lock will percolate through.
> > Do you mean the below implementation has live-lock risk?
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + u32 old, new, val = atomic_read(>val);
> > +
> > + for (;;) {
> > + new = (val & _Q_LOCKED_PENDING_MASK) | tail;
> > + old = atomic_cmpxchg(>val, val, new);
> > + if (old == val)
> > + break;
> > +
> > + val = old;
> > + }
> > + return old;
> > +}
> If there is a continuous stream of incoming spinlock takers, it is
> possible that some cpus may have to wait a long time to set the tail
> right. However, this should only happen on artificial workload. I doubt
> it will happen with real workload or with limit number of cpus.
Yes, I agree or it couldn't with NR_CPU > 16k.
So the implementation above is suitable for non-sub-word-xchg architecture.
> >
> >> Step 1 would be to get your architecute fixed such that it can provide
> >> fwd progress guarantees for LL/SC. Otherwise there's absolutely no point
> >> in building complex systems with it.
> > Quote Waiman's comment [1] on xchg16 optimization:
> >
> > "This optimization is needed to make the qspinlock achieve performance
> > parity with ticket spinlock at light load."
> >
> > [1]
> > https://lore.kernel.org/kvm/1429901803-29771-6-git-send-email-waiman.l...@hp.com/
> >
> > So for a non-xhg16 machine:
> > - ticket-lock for small numbers of CPUs
> > - qspinlock for large numbers of CPUs
> >
> > Okay, I'll put all of them into the next patch :P
> >
> It is true that qspinlock may not offer much advantage when the number
> of cpus is small. It shines for systems with many cpus. You may use
> NR_CPUS to determine if the default should be ticket or qspinlock with
> user override. To determine the right NR_CPUS threshold, you may need to
> run on real SMP RISCV systems to find out.
We'd give the choice to the users, and they could select ticket-lock
or qspinlock in riscv.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Hi Arnd On Wed, Mar 31, 2021 at 12:18 PM Guo Ren wrote: > > On Tue, Mar 30, 2021 at 3:12 PM Arnd Bergmann wrote: > > > > On Tue, Mar 30, 2021 at 4:26 AM Guo Ren wrote: > > > On Mon, Mar 29, 2021 at 9:56 PM Arnd Bergmann wrote: > > > > On Mon, Mar 29, 2021 at 2:52 PM Guo Ren wrote: > > > > > On Mon, Mar 29, 2021 at 7:31 PM Peter Zijlstra > > > > > wrote: > > > > > > > > > > > > What's the architectural guarantee on LL/SC progress for RISC-V ? > > > > > > > >"When LR/SC is used for memory locations marked RsrvNonEventual, > > > > software should provide alternative fall-back mechanisms used when > > > > lack of progress is detected." > > > > > > > > My reading of this is that if the example you tried stalls, then either > > > > the PMA is not RsrvEventual, and it is wrong to rely on ll/sc on this, > > > > or that the PMA is marked RsrvEventual but the implementation is > > > > buggy. > > > > > > Yes, PMA just defines physical memory region attributes, But in our > > > processor, when MMU is enabled (satp's value register > 2) in s-mode, > > > it will look at our custom PTE's attributes BIT(63) ref [1]: > > > > > >PTE format: > > >| 63 | 62 | 61 | 60 | 59 | 58-8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 > > > SO CBSH SERSW D A G U X W R V > > > ^^^^^ > > >BIT(63): SO - Strong Order > > >BIT(62): C - Cacheable > > >BIT(61): B - Bufferable > > >BIT(60): SH - Shareable > > >BIT(59): SE - Security > > > > > > So the memory also could be RsrvNone/RsrvEventual. > > > > I was not talking about RsrvNone, which would clearly mean that > > you cannot use lr/sc at all (trap would trap, right?), but > > "RsrvNonEventual", > > which would explain the behavior you described in an earlier reply: > > > > | u32 a = 0x55aa66bb; > > | u16 *ptr = > > | > > | CPU0 CPU1 > > | = = > > | xchg16(ptr, new) while(1) > > | WRITE_ONCE(*(ptr + 1), x); > > | > > | When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock. > > > > As I understand, this example must not cause a deadlock on > > a compliant hardware implementation when the underlying memory > > has RsrvEventual behavior, but could deadlock in case of > > RsrvNonEventual > Thx for the nice explanation: > - RsrvNonEventual - depends on software fall-back mechanisms, and > just I'm worried about. > - RsrvEventual - HW would provide the eventual success guarantee. In riscv-spec 8.3 Eventual Success of Store-Conditional Instructions I found: "As a consequence of the eventuality guarantee, if some harts in an execution environment are executing constrained LR/SC loops, and no other harts or devices in the execution environment execute an unconditional store or AMO to that reservation set, then at least one hart will eventually exit its constrained LR/SC loop. *** By contrast, if other harts or devices continue to write to that reservation set, it ***is not guaranteed*** that any hart will exit its LR/SC loop.*** " Seems RsrvEventual couldn't solve the code's problem I've mentioned. > > > > > > [1] > > > https://github.com/c-sky/csky-linux/commit/e837aad23148542771794d8a2fcc52afd0fcbf88 > > > > > > > > > > > It also seems that the current "amoswap" based implementation > > > > would be reliable independent of RsrvEventual/RsrvNonEventual. > > > > > > Yes, the hardware implementation of AMO could be different from LR/SC. > > > AMO could use ACE snoop holding to lock the bus in hw coherency > > > design, but LR/SC uses an exclusive monitor without locking the bus. > > > > > > RISC-V hasn't CAS instructions, and it uses LR/SC for cmpxchg. I don't > > > think LR/SC would be slower than CAS, and CAS is just good for code > > > size. > > > > What I meant here is that the current spinlock uses a simple amoswap, > > which presumably does not suffer from the lack of forward process you > > described. > Does that mean we should prevent using LR/SC (if RsrvNonEventual)? > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 3:12 PM Arnd Bergmann wrote: > > On Tue, Mar 30, 2021 at 4:26 AM Guo Ren wrote: > > On Mon, Mar 29, 2021 at 9:56 PM Arnd Bergmann wrote: > > > On Mon, Mar 29, 2021 at 2:52 PM Guo Ren wrote: > > > > On Mon, Mar 29, 2021 at 7:31 PM Peter Zijlstra > > > > wrote: > > > > > > > > > > What's the architectural guarantee on LL/SC progress for RISC-V ? > > > > > >"When LR/SC is used for memory locations marked RsrvNonEventual, > > > software should provide alternative fall-back mechanisms used when > > > lack of progress is detected." > > > > > > My reading of this is that if the example you tried stalls, then either > > > the PMA is not RsrvEventual, and it is wrong to rely on ll/sc on this, > > > or that the PMA is marked RsrvEventual but the implementation is > > > buggy. > > > > Yes, PMA just defines physical memory region attributes, But in our > > processor, when MMU is enabled (satp's value register > 2) in s-mode, > > it will look at our custom PTE's attributes BIT(63) ref [1]: > > > >PTE format: > >| 63 | 62 | 61 | 60 | 59 | 58-8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 > > SO CBSH SERSW D A G U X W R V > > ^^^^^ > >BIT(63): SO - Strong Order > >BIT(62): C - Cacheable > >BIT(61): B - Bufferable > >BIT(60): SH - Shareable > >BIT(59): SE - Security > > > > So the memory also could be RsrvNone/RsrvEventual. > > I was not talking about RsrvNone, which would clearly mean that > you cannot use lr/sc at all (trap would trap, right?), but "RsrvNonEventual", > which would explain the behavior you described in an earlier reply: > > | u32 a = 0x55aa66bb; > | u16 *ptr = > | > | CPU0 CPU1 > | = = > | xchg16(ptr, new) while(1) > | WRITE_ONCE(*(ptr + 1), x); > | > | When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock. > > As I understand, this example must not cause a deadlock on > a compliant hardware implementation when the underlying memory > has RsrvEventual behavior, but could deadlock in case of > RsrvNonEventual Thx for the nice explanation: - RsrvNonEventual - depends on software fall-back mechanisms, and just I'm worried about. - RsrvEventual - HW would provide the eventual success guarantee. > > > [1] > > https://github.com/c-sky/csky-linux/commit/e837aad23148542771794d8a2fcc52afd0fcbf88 > > > > > > > > It also seems that the current "amoswap" based implementation > > > would be reliable independent of RsrvEventual/RsrvNonEventual. > > > > Yes, the hardware implementation of AMO could be different from LR/SC. > > AMO could use ACE snoop holding to lock the bus in hw coherency > > design, but LR/SC uses an exclusive monitor without locking the bus. > > > > RISC-V hasn't CAS instructions, and it uses LR/SC for cmpxchg. I don't > > think LR/SC would be slower than CAS, and CAS is just good for code > > size. > > What I meant here is that the current spinlock uses a simple amoswap, > which presumably does not suffer from the lack of forward process you > described. Does that mean we should prevent using LR/SC (if RsrvNonEventual)? -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 12:54 PM Anup Patel wrote:
>
>
>
> > -Original Message-----
> > From: Guo Ren
> > Sent: 30 March 2021 08:44
> > To: Peter Zijlstra
> > Cc: linux-riscv ; Linux Kernel Mailing List
> > ; linux-c...@vger.kernel.org; linux-arch
> > ; Guo Ren ; Will
> > Deacon ; Ingo Molnar ; Waiman
> > Long ; Arnd Bergmann ; Anup
> > Patel
> > Subject: Re: [PATCH v4 3/4] locking/qspinlock: Add
> > ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> >
> > On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra
> > wrote:
> > >
> > > On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote:
> > > > u32 a = 0x55aa66bb;
> > > > u16 *ptr =
> > > >
> > > > CPU0 CPU1
> > > > = =
> > > > xchg16(ptr, new) while(1)
> > > > WRITE_ONCE(*(ptr + 1), x);
> > > >
> > > > When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock.
> > >
> > > Then I think your LL/SC is broken.
> > >
> > > That also means you really don't want to build super complex locking
> > > primitives on top, because that live-lock will percolate through.
> > Do you mean the below implementation has live-lock risk?
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + u32 old, new, val = atomic_read(>val);
> > +
> > + for (;;) {
> > + new = (val & _Q_LOCKED_PENDING_MASK) | tail;
> > + old = atomic_cmpxchg(>val, val, new);
> > + if (old == val)
> > + break;
> > +
> > + val = old;
> > + }
> > + return old;
> > +}
> >
> >
> > >
> > > Step 1 would be to get your architecute fixed such that it can provide
> > > fwd progress guarantees for LL/SC. Otherwise there's absolutely no
> > > point in building complex systems with it.
> >
> > Quote Waiman's comment [1] on xchg16 optimization:
> >
> > "This optimization is needed to make the qspinlock achieve performance
> > parity with ticket spinlock at light load."
> >
> > [1] https://lore.kernel.org/kvm/1429901803-29771-6-git-send-email-
> > waiman.l...@hp.com/
> >
> > So for a non-xhg16 machine:
> > - ticket-lock for small numbers of CPUs
> > - qspinlock for large numbers of CPUs
> >
> > Okay, I'll put all of them into the next patch
>
> I would suggest to have separate Kconfig opitons for ticket spinlock
> in Linux RISC-V which will be disabled by default. This means Linux
> RISC-V will use qspinlock by default and use ticket spinlock only when
> ticket spinlock kconfig is enabled.
OK
>
> Regards,
> Anup
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Tue, Mar 30, 2021 at 1:51 PM Anup Patel wrote: > > On Tue, Mar 30, 2021 at 7:56 AM Guo Ren wrote: > > > > On Mon, Mar 29, 2021 at 9:56 PM Arnd Bergmann wrote: > > > > > > On Mon, Mar 29, 2021 at 2:52 PM Guo Ren wrote: > > > > > > > > On Mon, Mar 29, 2021 at 7:31 PM Peter Zijlstra > > > > wrote: > > > > > > > > > > On Mon, Mar 29, 2021 at 01:16:53PM +0200, Peter Zijlstra wrote: > > > > > > Anyway, an additional 'funny' is that I suspect you cannot prove fwd > > > > > > progress of the entire primitive with any of this on. But who cares > > > > > > about details anyway.. :/ > > > > > > > > > > What's the architectural guarantee on LL/SC progress for RISC-V ? > > > > > > > > funct5| aq | rl | rs2 | rs1 | funct3 | rd | opcode > > > > 5 11 5 5 35 7 > > > > LR.W/D ordering 0 addrwidth destAMO > > > > SC.W/D ordering src addrwidth destAMO > > > > > > > > LR.W loads a word from the address in rs1, places the sign-extended > > > > value in rd, and registers a reservation set—a set of bytes that > > > > subsumes the bytes in the addressed word. SC.W conditionally writes a > > > > word in rs2 to the address in rs1: the SC.W succeeds only if the > > > > reservation is still valid and the reservation set contains the bytes > > > > being written. If the SC.W succeeds, the instruction writes the word > > > > in rs2 to memory, and it writes zero to rd. If the SC.W fails, the > > > > instruction does not write to memory, and it writes a nonzero value to > > > > rd. Regardless of success or failure, executing an SC.W instruction > > > > *invalidates any reservation held by this hart*. > > > > > > > > More details, ref: > > > > https://github.com/riscv/riscv-isa-manual > > > > > > I think section "3.5.3.2 Reservability PMA" [1] would be a more relevant > > > link, as this defines memory areas that either do or do not have > > > forward progress guarantees, including this part: > > > > > >"When LR/SC is used for memory locations marked RsrvNonEventual, > > > software should provide alternative fall-back mechanisms used when > > > lack of progress is detected." > > > > > > My reading of this is that if the example you tried stalls, then either > > > the PMA is not RsrvEventual, and it is wrong to rely on ll/sc on this, > > > or that the PMA is marked RsrvEventual but the implementation is > > > buggy. > > Yes, PMA just defines physical memory region attributes, But in our > > processor, when MMU is enabled (satp's value register > 2) in s-mode, > > it will look at our custom PTE's attributes BIT(63) ref [1]: > > > >PTE format: > >| 63 | 62 | 61 | 60 | 59 | 58-8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 > > SO CBSH SERSW D A G U X W R V > > ^^^^^ > >BIT(63): SO - Strong Order > >BIT(62): C - Cacheable > >BIT(61): B - Bufferable > >BIT(60): SH - Shareable > >BIT(59): SE - Security > > > > So the memory also could be RsrvNone/RsrvEventual. > > > > [1] > > https://github.com/c-sky/csky-linux/commit/e837aad23148542771794d8a2fcc52afd0fcbf88 > > Is this about your C-sky architecture or your RISC-V implementation. It's in RISC-V implementation. > > If these PTE bits are in your RISC-V implementation then clearly your > RISC-V implementation is not compliant with the RISC-V privilege spec > because these bits are not defined in RISC-V privilege spec. We could disable it if the vendor's SOC has a coherency interconnect bus, so C910 is compliant with standard privilege spec. ps: I remember someone has mentioned a similar design in 1.12-draft-VM-TASKGROUP: "Bit 63 indicates that the PTE uses a custom implementation-specific encoding. If bit 63 is set, the algorithm for virtual-to-physical address translation is implementation-defined. If bit 63 is not set, the algorithm for virtual-to-physical address translation is described in Section 4.4.2. Bit 62 indicates the use of naturally aligned power-of-two (NAPOT) address translation contiguity, as described in Section 4.4.2. Bits 61–xx indicate cacheability attributes associated with the virtual address in question, as de-scribed in Section 4.4.3. Bits xx–54 are reserved for future use." > > Regards,
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 8:50 PM Peter Zijlstra wrote:
>
> On Mon, Mar 29, 2021 at 08:01:41PM +0800, Guo Ren wrote:
> > u32 a = 0x55aa66bb;
> > u16 *ptr =
> >
> > CPU0 CPU1
> > = =
> > xchg16(ptr, new) while(1)
> > WRITE_ONCE(*(ptr + 1), x);
> >
> > When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock.
>
> Then I think your LL/SC is broken.
>
> That also means you really don't want to build super complex locking
> primitives on top, because that live-lock will percolate through.
Do you mean the below implementation has live-lock risk?
+static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
+{
+ u32 old, new, val = atomic_read(>val);
+
+ for (;;) {
+ new = (val & _Q_LOCKED_PENDING_MASK) | tail;
+ old = atomic_cmpxchg(>val, val, new);
+ if (old == val)
+ break;
+
+ val = old;
+ }
+ return old;
+}
>
> Step 1 would be to get your architecute fixed such that it can provide
> fwd progress guarantees for LL/SC. Otherwise there's absolutely no point
> in building complex systems with it.
Quote Waiman's comment [1] on xchg16 optimization:
"This optimization is needed to make the qspinlock achieve performance
parity with ticket spinlock at light load."
[1]
https://lore.kernel.org/kvm/1429901803-29771-6-git-send-email-waiman.l...@hp.com/
So for a non-xhg16 machine:
- ticket-lock for small numbers of CPUs
- qspinlock for large numbers of CPUs
Okay, I'll put all of them into the next patch :P
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 9:56 PM Arnd Bergmann wrote: > > On Mon, Mar 29, 2021 at 2:52 PM Guo Ren wrote: > > > > On Mon, Mar 29, 2021 at 7:31 PM Peter Zijlstra wrote: > > > > > > On Mon, Mar 29, 2021 at 01:16:53PM +0200, Peter Zijlstra wrote: > > > > Anyway, an additional 'funny' is that I suspect you cannot prove fwd > > > > progress of the entire primitive with any of this on. But who cares > > > > about details anyway.. :/ > > > > > > What's the architectural guarantee on LL/SC progress for RISC-V ? > > > > funct5| aq | rl | rs2 | rs1 | funct3 | rd | opcode > > 5 11 5 5 35 7 > > LR.W/D ordering 0 addrwidth destAMO > > SC.W/D ordering src addrwidth destAMO > > > > LR.W loads a word from the address in rs1, places the sign-extended > > value in rd, and registers a reservation set—a set of bytes that > > subsumes the bytes in the addressed word. SC.W conditionally writes a > > word in rs2 to the address in rs1: the SC.W succeeds only if the > > reservation is still valid and the reservation set contains the bytes > > being written. If the SC.W succeeds, the instruction writes the word > > in rs2 to memory, and it writes zero to rd. If the SC.W fails, the > > instruction does not write to memory, and it writes a nonzero value to > > rd. Regardless of success or failure, executing an SC.W instruction > > *invalidates any reservation held by this hart*. > > > > More details, ref: > > https://github.com/riscv/riscv-isa-manual > > I think section "3.5.3.2 Reservability PMA" [1] would be a more relevant > link, as this defines memory areas that either do or do not have > forward progress guarantees, including this part: > >"When LR/SC is used for memory locations marked RsrvNonEventual, > software should provide alternative fall-back mechanisms used when > lack of progress is detected." > > My reading of this is that if the example you tried stalls, then either > the PMA is not RsrvEventual, and it is wrong to rely on ll/sc on this, > or that the PMA is marked RsrvEventual but the implementation is > buggy. Yes, PMA just defines physical memory region attributes, But in our processor, when MMU is enabled (satp's value register > 2) in s-mode, it will look at our custom PTE's attributes BIT(63) ref [1]: PTE format: | 63 | 62 | 61 | 60 | 59 | 58-8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 SO CBSH SERSW D A G U X W R V ^^^^^ BIT(63): SO - Strong Order BIT(62): C - Cacheable BIT(61): B - Bufferable BIT(60): SH - Shareable BIT(59): SE - Security So the memory also could be RsrvNone/RsrvEventual. [1] https://github.com/c-sky/csky-linux/commit/e837aad23148542771794d8a2fcc52afd0fcbf88 > > It also seems that the current "amoswap" based implementation > would be reliable independent of RsrvEventual/RsrvNonEventual. Yes, the hardware implementation of AMO could be different from LR/SC. AMO could use ACE snoop holding to lock the bus in hw coherency design, but LR/SC uses an exclusive monitor without locking the bus. > arm64 is already in the situation of having to choose between > two cmpxchg() implementation at runtime to allow falling back to > a slower but more general version, but it's best to avoid that if you > can. Current RISC-V needn't multiple versions to select, and all AMO & LR/SC has been defined in the spec. RISC-V hasn't CAS instructions, and it uses LR/SC for cmpxchg. I don't think LR/SC would be slower than CAS, and CAS is just good for code size. > > Arnd > > [1] > http://www.five-embeddev.com/riscv-isa-manual/latest/machine.html#atomicity-pmas -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 7:31 PM Peter Zijlstra wrote: > > On Mon, Mar 29, 2021 at 01:16:53PM +0200, Peter Zijlstra wrote: > > Anyway, an additional 'funny' is that I suspect you cannot prove fwd > > progress of the entire primitive with any of this on. But who cares > > about details anyway.. :/ > > What's the architectural guarantee on LL/SC progress for RISC-V ? funct5| aq | rl | rs2 | rs1 | funct3 | rd | opcode 5 11 5 5 35 7 LR.W/D ordering 0 addrwidth destAMO SC.W/D ordering src addrwidth destAMO LR.W loads a word from the address in rs1, places the sign-extended value in rd, and registers a reservation set—a set of bytes that subsumes the bytes in the addressed word. SC.W conditionally writes a word in rs2 to the address in rs1: the SC.W succeeds only if the reservation is still valid and the reservation set contains the bytes being written. If the SC.W succeeds, the instruction writes the word in rs2 to memory, and it writes zero to rd. If the SC.W fails, the instruction does not write to memory, and it writes a nonzero value to rd. Regardless of success or failure, executing an SC.W instruction *invalidates any reservation held by this hart*. More details, ref: https://github.com/riscv/riscv-isa-manual > And what if you double loop it like cmpxchg() ? Can you give a code snippet? -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 7:26 PM Peter Zijlstra wrote: > > On Mon, Mar 29, 2021 at 07:19:29PM +0800, Guo Ren wrote: > > On Mon, Mar 29, 2021 at 3:50 PM Peter Zijlstra wrote: > > > > > > On Sat, Mar 27, 2021 at 06:06:38PM +, guo...@kernel.org wrote: > > > > From: Guo Ren > > > > > > > > Some architectures don't have sub-word swap atomic instruction, > > > > they only have the full word's one. > > > > > > > > The sub-word swap only improve the performance when: > > > > NR_CPUS < 16K > > > > * 0- 7: locked byte > > > > * 8: pending > > > > * 9-15: not used > > > > * 16-17: tail index > > > > * 18-31: tail cpu (+1) > > > > > > > > The 9-15 bits are wasted to use xchg16 in xchg_tail. > > > > > > > > Please let architecture select xchg16/xchg32 to implement > > > > xchg_tail. > > > > > > So I really don't like this, this pushes complexity into the generic > > > code for something that's really not needed. > > > > > > Lots of RISC already implement sub-word atomics using word ll/sc. > > > Obviously they're not sharing code like they should be :/ See for > > > example arch/mips/kernel/cmpxchg.c. > > I see, we've done two versions of this: > > - Using cmpxchg codes from MIPS by Michael > > - Re-write with assembly codes by Guo > > > > But using the full-word atomic xchg instructions implement xchg16 has > > the semantic risk for atomic operations. > > What? -ENOPARSE u32 a = 0x55aa66bb; u16 *ptr = CPU0 CPU1 = = xchg16(ptr, new) while(1) WRITE_ONCE(*(ptr + 1), x); When we use lr.w/sc.w implement xchg16, it'll cause CPU0 deadlock. > > > > Also, I really do think doing ticket locks first is a far more sensible > > > step. > > NACK by Anup > > Who's he when he's not sending NAKs ? We've talked before: https://lore.kernel.org/linux-riscv/caahsdy1jhlufwu7rucaq+ruwrbks2ksdva7eprt8--4zfof...@mail.gmail.com/T/#t -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Mon, Mar 29, 2021 at 3:50 PM Peter Zijlstra wrote: > > On Sat, Mar 27, 2021 at 06:06:38PM +, guo...@kernel.org wrote: > > From: Guo Ren > > > > Some architectures don't have sub-word swap atomic instruction, > > they only have the full word's one. > > > > The sub-word swap only improve the performance when: > > NR_CPUS < 16K > > * 0- 7: locked byte > > * 8: pending > > * 9-15: not used > > * 16-17: tail index > > * 18-31: tail cpu (+1) > > > > The 9-15 bits are wasted to use xchg16 in xchg_tail. > > > > Please let architecture select xchg16/xchg32 to implement > > xchg_tail. > > So I really don't like this, this pushes complexity into the generic > code for something that's really not needed. > > Lots of RISC already implement sub-word atomics using word ll/sc. > Obviously they're not sharing code like they should be :/ See for > example arch/mips/kernel/cmpxchg.c. I see, we've done two versions of this: - Using cmpxchg codes from MIPS by Michael - Re-write with assembly codes by Guo But using the full-word atomic xchg instructions implement xchg16 has the semantic risk for atomic operations. I don't think export xchg16 in a none-sub-word atomic machine is correct. > > Also, I really do think doing ticket locks first is a far more sensible > step. NACK by Anup -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v5 4/7] powerpc/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Sun, Mar 28, 2021 at 7:14 PM Christophe Leroy wrote: > > > > Le 28/03/2021 à 08:30, guo...@kernel.org a écrit : > > From: Guo Ren > > > > We don't have native hw xchg16 instruction, so let qspinlock > > generic code to deal with it. > > We have lharx/sthcx pair on some versions of powerpc. > > See > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20201107032328.2454582-1-npig...@gmail.com/ Got it, thx for the information. > > Christophe > > > > > Using the full-word atomic xchg instructions implement xchg16 has > > the semantic risk for atomic operations. > > > > This patch cancels the dependency of on qspinlock generic code on > > architecture's xchg16. > > > > Signed-off-by: Guo Ren > > Cc: Michael Ellerman > > Cc: Benjamin Herrenschmidt > > Cc: Paul Mackerras > > --- > > arch/powerpc/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > index 386ae12d8523..69ec4ade6521 100644 > > --- a/arch/powerpc/Kconfig > > +++ b/arch/powerpc/Kconfig > > @@ -151,6 +151,7 @@ config PPC > > select ARCH_USE_CMPXCHG_LOCKREF if PPC64 > > select ARCH_USE_QUEUED_RWLOCKS if PPC_QUEUED_SPINLOCKS > > select ARCH_USE_QUEUED_SPINLOCKSif PPC_QUEUED_SPINLOCKS > > + select ARCH_USE_QUEUED_SPINLOCKS_XCHG32 if PPC_QUEUED_SPINLOCKS > > select ARCH_WANT_IPC_PARSE_VERSION > > select ARCH_WANT_IRQS_OFF_ACTIVATE_MM > > select ARCH_WANT_LD_ORPHAN_WARN > > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 2/4] riscv: cmpxchg.h: Merge macros
Thx Arnd, On Sun, Mar 28, 2021 at 5:25 AM Arnd Bergmann wrote: > > On Sat, Mar 27, 2021 at 7:06 PM wrote: > > > > From: Guo Ren > > > > To reduce assembly codes, let's merge duplicate codes into one > > (xchg_acquire, xchg_release, cmpxchg_release). > > > > Signed-off-by: Guo Ren > > This is a nice cleanup, but I wonder if you can go even further by using > the definitions from atomic-arch-fallback.h like arm64 and x86 do. Ok, I'll separate it from the qspinlock patchset and try atomic-arch-fallback.h. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 3/4] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
On Sun, Mar 28, 2021 at 2:43 AM Waiman Long wrote:
>
> On 3/27/21 2:06 PM, guo...@kernel.org wrote:
> > From: Guo Ren
> >
> > Some architectures don't have sub-word swap atomic instruction,
> > they only have the full word's one.
> >
> > The sub-word swap only improve the performance when:
> > NR_CPUS < 16K
> > * 0- 7: locked byte
> > * 8: pending
> > * 9-15: not used
> > * 16-17: tail index
> > * 18-31: tail cpu (+1)
> >
> > The 9-15 bits are wasted to use xchg16 in xchg_tail.
> >
> > Please let architecture select xchg16/xchg32 to implement
> > xchg_tail.
> >
> > Signed-off-by: Guo Ren
> > Cc: Peter Zijlstra
> > Cc: Will Deacon
> > Cc: Ingo Molnar
> > Cc: Waiman Long
> > Cc: Arnd Bergmann
> > Cc: Anup Patel
> > ---
> > kernel/Kconfig.locks | 3 +++
> > kernel/locking/qspinlock.c | 44 +-
> > 2 files changed, 27 insertions(+), 20 deletions(-)
> >
> > diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
> > index 3de8fd11873b..d02f1261f73f 100644
> > --- a/kernel/Kconfig.locks
> > +++ b/kernel/Kconfig.locks
> > @@ -239,6 +239,9 @@ config LOCK_SPIN_ON_OWNER
> > config ARCH_USE_QUEUED_SPINLOCKS
> > bool
> >
> > +config ARCH_USE_QUEUED_SPINLOCKS_XCHG32
> > + bool
> > +
> > config QUEUED_SPINLOCKS
> > def_bool y if ARCH_USE_QUEUED_SPINLOCKS
> > depends on SMP
> > diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> > index cbff6ba53d56..54de0632c6a8 100644
> > --- a/kernel/locking/qspinlock.c
> > +++ b/kernel/locking/qspinlock.c
> > @@ -163,26 +163,6 @@ static __always_inline void
> > clear_pending_set_locked(struct qspinlock *lock)
> > WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
> > }
> >
> > -/*
> > - * xchg_tail - Put in the new queue tail code word & retrieve previous one
> > - * @lock : Pointer to queued spinlock structure
> > - * @tail : The new queue tail code word
> > - * Return: The previous queue tail code word
> > - *
> > - * xchg(lock, tail), which heads an address dependency
> > - *
> > - * p,*,* -> n,*,* ; prev = xchg(lock, node)
> > - */
> > -static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > -{
> > - /*
> > - * We can use relaxed semantics since the caller ensures that the
> > - * MCS node is properly initialized before updating the tail.
> > - */
> > - return (u32)xchg_relaxed(>tail,
> > - tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
> > -}
> > -
> > #else /* _Q_PENDING_BITS == 8 */
> >
> > /**
> > @@ -206,6 +186,30 @@ static __always_inline void
> > clear_pending_set_locked(struct qspinlock *lock)
> > {
> > atomic_add(-_Q_PENDING_VAL + _Q_LOCKED_VAL, >val);
> > }
> > +#endif
> > +
> > +#if _Q_PENDING_BITS == 8 &&
> > !defined(CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32)
> > +/*
> > + * xchg_tail - Put in the new queue tail code word & retrieve previous one
> > + * @lock : Pointer to queued spinlock structure
> > + * @tail : The new queue tail code word
> > + * Return: The previous queue tail code word
> > + *
> > + * xchg(lock, tail), which heads an address dependency
> > + *
> > + * p,*,* -> n,*,* ; prev = xchg(lock, node)
> > + */
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + /*
> > + * We can use relaxed semantics since the caller ensures that the
> > + * MCS node is properly initialized before updating the tail.
> > + */
> > + return (u32)xchg_relaxed(>tail,
> > + tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
> > +}
> > +
> > +#else
> >
> > /**
> >* xchg_tail - Put in the new queue tail code word & retrieve previous one
>
> I don't have any problem adding a
> CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32 config option to control that.
Thx
>
> One minor nit:
>
> #endif /* _Q_PENDING_BITS == 8 */
>
> You should probably remove the comment at the trailing end of the
> corresponding "#endif" as it is now wrong.
I'll fix it in next patch
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v3 4/4] riscv: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 5ca41152cf4b..894e170c503e 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -17,6 +17,14 @@
#define __local_release_fence()
\
__asm__ __volatile__(RISCV_RELEASE_BARRIER "" ::: "memory")
+#ifdef CONFIG_32BIT
+#define __ASM_SLLIW "slli\t"
+#define __ASM_SRLIW "srli\t"
+#else
+#define __ASM_SLLIW "slliw\t"
+#define __ASM_SRLIW "srliw\t"
+#endif
+
#define __xchg_relaxed(ptr, new, size) \
({ \
__typeof__(ptr) __ptr = (ptr); \
@@ -31,14 +39,14 @@
__asm__ __volatile__ ( \
"0: lr.w%0, (%4)\n" \
" mv %1, %0 \n" \
- " slliw %1, %1, 16 \n" \
- " srliw %1, %1, 16 \n" \
+ __ASM_SLLIW"%1, %1, 16 \n" \
+ __ASM_SRLIW"%1, %1, 16 \n" \
" mv %2, %3 \n" \
- " slliw %2, %2, 16 \n" \
+ __ASM_SLLIW"%2, %2, 16 \n" \
" or %1, %2, %1 \n" \
" sc.w%2, %1, (%4)\n" \
" bnez%2, 0b \n" \
- " srliw %0, %0, 16 \n" \
+ __ASM_SRLIW"%0, %0, 16 \n" \
: "=" (__ret), "=" (tmp), "=" (__rc) \
: "r" (__new), "r"(addr)\
: "memory");\
@@ -46,14 +54,14 @@
__asm__ __volatile__ ( \
"0: lr.w%0, (%4)\n" \
" mv %1, %0 \n" \
- " srliw %1, %1, 16 \n" \
- " slliw %1, %1, 16 \n" \
+ __ASM_SRLIW"%1, %1, 16 \n" \
+ __ASM_SLLIW"%1, %1, 16 \n" \
" mv %2, %3 \n" \
" or %1, %2, %1 \n" \
" sc.w%2, %1, 0(%4) \n" \
" bnez%2, 0b \n" \
- " slliw %0, %0, 16 \n" \
- " srliw %0, %0, 16 \n" \
+ __ASM_SLLIW"%0, %0, 16 \n" \
+ __ASM_SRLIW"%0, %0, 16 \n" \
: "=" (__ret), "=" (tmp), "=" (__rc) \
: "r" (__new), "r"(addr)\
: "memory");\
On Thu, Mar 25, 2021 at 7:34 PM Guo Ren wrote:
>
> haha, I forgot RV32, it needs a
>
> #ifdef RV32
> srliw
> #else
> srli
> #endif
>
> On Thu, Mar 25, 2021 at 7:16 PM kernel test robot wrote:
> >
> > Hi,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on tip/locking/core]
> > [also build test ERROR on linux/master linus/master v5.12-rc4 next-20210325]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url:
> > https://github.com/0day-ci/linux/commits/guoren-kernel-org/riscv-Add-qspinlock-qrwlock/20210325-155933
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
> > 5965a7adbd72dd9b288c0911cb73719fed1efa08
> > config: riscv-rv32_defconfig (attached as .config)
> > compiler: riscv32-linux-gcc (GCC) 9.3.0
> > reproduce (this is a W=1 build):
> >
Re: [PATCH v3 4/4] riscv: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
haha, I forgot RV32, it needs a
#ifdef RV32
srliw
#else
srli
#endif
On Thu, Mar 25, 2021 at 7:16 PM kernel test robot wrote:
>
> Hi,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tip/locking/core]
> [also build test ERROR on linux/master linus/master v5.12-rc4 next-20210325]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
> https://github.com/0day-ci/linux/commits/guoren-kernel-org/riscv-Add-qspinlock-qrwlock/20210325-155933
> base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
> 5965a7adbd72dd9b288c0911cb73719fed1efa08
> config: riscv-rv32_defconfig (attached as .config)
> compiler: riscv32-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> #
> https://github.com/0day-ci/linux/commit/2bf2d117ab34b007089e20e1c46eff89b5da097e
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> guoren-kernel-org/riscv-Add-qspinlock-qrwlock/20210325-155933
> git checkout 2bf2d117ab34b007089e20e1c46eff89b5da097e
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
> ARCH=riscv
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All errors (new ones prefixed by >>):
>
>kernel/locking/qspinlock.c: Assembler messages:
>kernel/locking/qspinlock.c:184: Error: unrecognized opcode `srliw t1,t1,16'
>kernel/locking/qspinlock.c:185: Error: unrecognized opcode `slliw t1,t1,16'
> >> kernel/locking/qspinlock.c:190: Error: unrecognized opcode `slliw a6,a6,16'
> >> kernel/locking/qspinlock.c:191: Error: unrecognized opcode `srliw a6,a6,16'
>kernel/locking/qspinlock.c:184: Error: unrecognized opcode `slliw t1,t1,16'
>kernel/locking/qspinlock.c:185: Error: unrecognized opcode `srliw t1,t1,16'
>kernel/locking/qspinlock.c:187: Error: unrecognized opcode `slliw t3,t3,16'
> >> kernel/locking/qspinlock.c:191: Error: unrecognized opcode `srliw a6,a6,16'
>
>
> vim +190 kernel/locking/qspinlock.c
>
> 69f9cae90907e0 Peter Zijlstra (Intel 2015-04-24 187)
> 59fb586b4a07b4 Will Deacon2018-04-26 188 /**
> 59fb586b4a07b4 Will Deacon2018-04-26 189 * clear_pending -
> clear the pending bit.
> 59fb586b4a07b4 Will Deacon2018-04-26 @190 * @lock: Pointer to
> queued spinlock structure
> 59fb586b4a07b4 Will Deacon2018-04-26 @191 *
> 59fb586b4a07b4 Will Deacon2018-04-26 192 * *,1,* -> *,0,*
> 59fb586b4a07b4 Will Deacon2018-04-26 193 */
> 59fb586b4a07b4 Will Deacon2018-04-26 194 static __always_inline
> void clear_pending(struct qspinlock *lock)
> 59fb586b4a07b4 Will Deacon2018-04-26 195 {
> 59fb586b4a07b4 Will Deacon2018-04-26 196
> atomic_andnot(_Q_PENDING_VAL, >val);
> 59fb586b4a07b4 Will Deacon 2018-04-26 197 }
> 59fb586b4a07b4 Will Deacon2018-04-26 198
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
On Wed, Mar 24, 2021 at 7:16 PM Vitaly Wool wrote:
>
>
>
> On Wed, Mar 24, 2021, 11:16 AM wrote:
>>
>> From: Guo Ren
>>
>> This patch introduces a ticket lock implementation for riscv, along the
>> same lines as the implementation for arch/arm & arch/csky.
>
>
> Could you please provide a rationale for this? Like, what is wrong with the
> current implementation.
Ticket based spinlock's principle is here:
https://lwn.net/Articles/267968/
Current implementation will cause cache line bouncing when many harts
are acquiring the same spinlock.
I'm seeking a solution, maybe not fitting the current RISC-V base ISA.
I'll add more comments in the next version of patch.
>
> Thanks in advance,
>
> Best regards,
>Vitaly
>>
>>
>> Signed-off-by: Guo Ren
>> Cc: Catalin Marinas
>> Cc: Will Deacon
>> Cc: Peter Zijlstra
>> Cc: Palmer Dabbelt
>> Cc: Anup Patel
>> Cc: Arnd Bergmann
>> ---
>> arch/riscv/Kconfig | 1 +
>> arch/riscv/include/asm/Kbuild | 1 +
>> arch/riscv/include/asm/spinlock.h | 158
>>
>> arch/riscv/include/asm/spinlock_types.h | 19 ++--
>> 4 files changed, 74 insertions(+), 105 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 87d7b52..7c56a20 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -30,6 +30,7 @@ config RISCV
>> select ARCH_HAS_STRICT_KERNEL_RWX if MMU
>> select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>> select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
>> + select ARCH_USE_QUEUED_RWLOCKS
>> select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
>> select ARCH_WANT_FRAME_POINTERS
>> select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
>> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
>> index 445ccc9..e57ef80 100644
>> --- a/arch/riscv/include/asm/Kbuild
>> +++ b/arch/riscv/include/asm/Kbuild
>> @@ -3,5 +3,6 @@ generic-y += early_ioremap.h
>> generic-y += extable.h
>> generic-y += flat.h
>> generic-y += kvm_para.h
>> +generic-y += qrwlock.h
>> generic-y += user.h
>> generic-y += vmlinux.lds.h
>> diff --git a/arch/riscv/include/asm/spinlock.h
>> b/arch/riscv/include/asm/spinlock.h
>> index f4f7fa1..2c81764 100644
>> --- a/arch/riscv/include/asm/spinlock.h
>> +++ b/arch/riscv/include/asm/spinlock.h
>> @@ -7,129 +7,91 @@
>> #ifndef _ASM_RISCV_SPINLOCK_H
>> #define _ASM_RISCV_SPINLOCK_H
>>
>> -#include
>> -#include
>> -#include
>> -
>> /*
>> - * Simple spin lock operations. These provide no fairness guarantees.
>> + * Ticket-based spin-locking.
>> */
>> +static inline void arch_spin_lock(arch_spinlock_t *lock)
>> +{
>> + arch_spinlock_t lockval;
>> + u32 tmp;
>> +
>> + asm volatile (
>> + "1: lr.w%0, %2 \n"
>> + " mv %1, %0 \n"
>> + " addw%0, %0, %3 \n"
>> + " sc.w%0, %0, %2 \n"
>> + " bnez%0, 1b \n"
>> + : "=" (tmp), "=" (lockval), "+A" (lock->lock)
>> + : "r" (1 << TICKET_NEXT)
>> + : "memory");
>>
>> -/* FIXME: Replace this with a ticket lock, like MIPS. */
>> -
>> -#define arch_spin_is_locked(x) (READ_ONCE((x)->lock) != 0)
>> + while (lockval.tickets.next != lockval.tickets.owner) {
>> + /*
>> +* FIXME - we need wfi/wfe here to prevent:
>> +* - cache line bouncing
>> +* - saving cpu pipeline in multi-harts-per-core
>> +*processor
>> +*/
>> + lockval.tickets.owner = READ_ONCE(lock->tickets.owner);
>> + }
>>
>> -static inline void arch_spin_unlock(arch_spinlock_t *lock)
>> -{
>> - smp_store_release(>lock, 0);
>> + __atomic_acquire_fence();
>> }
>>
>> static inline int arch_spin_trylock(arch_spinlock_t *lock)
>> {
>> - int tmp = 1, busy;
>> -
>> - __asm__ __volatile__ (
>> - " amoswap.w %0, %2, %1\n"
>> - RISCV_ACQUIRE_BARRIER
>> - : "=r" (busy), "+A" (lock->l
Re: [PATCH] riscv: locks: introduce ticket-based spinlock implementation
Thx Peter,
On Wed, Mar 24, 2021 at 7:09 PM Peter Zijlstra wrote:
>
> On Wed, Mar 24, 2021 at 10:14:52AM +, guo...@kernel.org wrote:
> > +static inline void arch_spin_lock(arch_spinlock_t *lock)
> > +{
> > + arch_spinlock_t lockval;
> > + u32 tmp;
> > +
> > + asm volatile (
> > + "1: lr.w%0, %2 \n"
> > + " mv %1, %0 \n"
> > + " addw%0, %0, %3 \n"
> > + " sc.w%0, %0, %2 \n"
> > + " bnez%0, 1b \n"
> > + : "=" (tmp), "=" (lockval), "+A" (lock->lock)
> > + : "r" (1 << TICKET_NEXT)
> > + : "memory");
> >
> > + while (lockval.tickets.next != lockval.tickets.owner) {
> > + /*
> > + * FIXME - we need wfi/wfe here to prevent:
> > + * - cache line bouncing
> > + * - saving cpu pipeline in multi-harts-per-core
> > + *processor
> > + */
> > + lockval.tickets.owner = READ_ONCE(lock->tickets.owner);
> > + }
> >
> > + __atomic_acquire_fence();
> > }
>
> > +static inline void arch_spin_unlock(arch_spinlock_t *lock)
> > {
> > + smp_store_release(>tickets.owner, lock->tickets.owner + 1);
> > + /* FIXME - we need ipi/sev here to notify above */
> > }
>
> Urgh, are you saying your WFE requires an explicit SEV like on ARM ? The
Yes, I'm considering that kind of code.
> ARM64 model is far superious IMO, and then you can use
> smp_cond_load_acquire() in arch_spin_lock() and call it a day.
Great tip, thx. I'll follow that.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: fix typos
Thx,
Looks good to me, Signed-off-by.
On Sat, Mar 6, 2021 at 11:23 AM angkery wrote:
>
> From: Junlin Yang
>
> fixes three typos found by codespell.
>
> Signed-off-by: Junlin Yang
> ---
> arch/csky/include/asm/asid.h| 2 +-
> arch/csky/include/asm/barrier.h | 2 +-
> arch/csky/include/asm/vdso.h| 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/csky/include/asm/asid.h b/arch/csky/include/asm/asid.h
> index ac08b0f..6ff205a 100644
> --- a/arch/csky/include/asm/asid.h
> +++ b/arch/csky/include/asm/asid.h
> @@ -37,7 +37,7 @@ void asid_new_context(struct asid_info *info, atomic64_t
> *pasid,
> * Check the ASID is still valid for the context. If not generate a new ASID.
> *
> * @pasid: Pointer to the current ASID batch
> - * @cpu: current CPU ID. Must have been acquired throught get_cpu()
> + * @cpu: current CPU ID. Must have been acquired through get_cpu()
> */
> static inline void asid_check_context(struct asid_info *info,
> atomic64_t *pasid, unsigned int cpu,
> diff --git a/arch/csky/include/asm/barrier.h b/arch/csky/include/asm/barrier.h
> index 84fc600c..f4045dd 100644
> --- a/arch/csky/include/asm/barrier.h
> +++ b/arch/csky/include/asm/barrier.h
> @@ -64,7 +64,7 @@
>
> /*
> * sync:completion barrier, all sync.xx instructions
> - * guarantee the last response recieved by bus transaction
> + * guarantee the last response received by bus transaction
> * made by ld/st instructions before sync.s
> * sync.s: inherit from sync, but also shareable to other cores
> * sync.i: inherit from sync, but also flush cpu pipeline
> diff --git a/arch/csky/include/asm/vdso.h b/arch/csky/include/asm/vdso.h
> index eb5142f..bdce581 100644
> --- a/arch/csky/include/asm/vdso.h
> +++ b/arch/csky/include/asm/vdso.h
> @@ -16,7 +16,7 @@ struct vdso_data {
> * offset of 0, but since the linker must support setting weak undefined
> * symbols to the absolute address 0 it also happens to support other low
> * addresses even when the code model suggests those low addresses would not
> - * otherwise be availiable.
> + * otherwise be available.
> */
> #define VDSO_SYMBOL(base, name)
> \
> ({
> \
> --
> 1.9.1
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] module: remove duplicate include in arch/csky/kernel/entry.S
Thx, looks good for me. On Wed, Mar 3, 2021 at 4:04 PM wrote: > > From: Zhang Yunkai > > 'asm/setup.h' included in 'arch/csky/kernel/entry.S' is duplicated. > > Signed-off-by: Zhang Yunkai > --- > arch/csky/kernel/entry.S | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/csky/kernel/entry.S b/arch/csky/kernel/entry.S > index c1bd7a6b4ab6..00e3c8ebf9b8 100644 > --- a/arch/csky/kernel/entry.S > +++ b/arch/csky/kernel/entry.S > @@ -9,7 +9,6 @@ > #include > #include > #include > -#include > #include > #include > > -- > 2.25.1 > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [GIT PULL] csky changes for v5.12-rc1
lockevents/csky/timer:starting",
csky_mptimer_starting_cpu,
csky_mptimer_dying_cpu);
- if (ret)
+ if (ret < 0)
return -EINVAL;
return 0;
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index f14adb8..5abcfda 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -134,7 +134,6 @@ enum cpuhp_state {
CPUHP_AP_ARC_TIMER_STARTING,
CPUHP_AP_RISCV_TIMER_STARTING,
CPUHP_AP_CLINT_TIMER_STARTING,
- CPUHP_AP_CSKY_TIMER_STARTING,
CPUHP_AP_HYPERV_TIMER_STARTING,
CPUHP_AP_KVM_STARTING,
CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
@@ -186,7 +185,6 @@ enum cpuhp_state {
CPUHP_AP_PERF_POWERPC_TRACE_IMC_ONLINE,
CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
CPUHP_AP_PERF_POWERPC_HV_GPCI_ONLINE,
- CPUHP_AP_PERF_CSKY_ONLINE,
CPUHP_AP_WATCHDOG_ONLINE,
CPUHP_AP_WORKQUEUE_ONLINE,
CPUHP_AP_RCUTREE_ONLINE,
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 2/2] Documentation: features: refresh feature list
Thx Arnd,
I'm very glad to see C-SKY & RISC-V features updated.
Signed-off-by: Guo Ren
On Thu, Feb 25, 2021 at 10:29 PM Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> Run the update script to document the recent feature additions
> on riscv, mips and csky.
>
> Fixes: c109f42450ec ("csky: Add kmemleak support")
> Fixes: 8b3165e54566 ("MIPS: Enable GCOV")
> Fixes: 1ddc96bd42da ("MIPS: kernel: Support extracting off-line stack traces
> from user-space with perf")
> Fixes: 74784081aac8 ("riscv: Add uprobes supported")
> Fixes: 829adda597fe ("riscv: Add KPROBES_ON_FTRACE supported")
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Fixes: dcdc7a53a890 ("RISC-V: Implement ptrace regs and stack API")
> Signed-off-by: Arnd Bergmann
> ---
> Documentation/features/debug/gcov-profile-all/arch-support.txt | 2 +-
> Documentation/features/debug/kmemleak/arch-support.txt | 2 +-
> Documentation/features/debug/kprobes-on-ftrace/arch-support.txt | 2 +-
> Documentation/features/debug/kprobes/arch-support.txt | 2 +-
> Documentation/features/debug/kretprobes/arch-support.txt| 2 +-
> Documentation/features/debug/uprobes/arch-support.txt | 2 +-
> Documentation/features/perf/kprobes-event/arch-support.txt | 2 +-
> Documentation/features/perf/perf-regs/arch-support.txt | 2 +-
> Documentation/features/perf/perf-stackdump/arch-support.txt | 2 +-
> Documentation/features/sched/numa-balancing/arch-support.txt| 2 +-
> 10 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/features/debug/gcov-profile-all/arch-support.txt
> b/Documentation/features/debug/gcov-profile-all/arch-support.txt
> index 416c70345946..b39c1a5de3f3 100644
> --- a/Documentation/features/debug/gcov-profile-all/arch-support.txt
> +++ b/Documentation/features/debug/gcov-profile-all/arch-support.txt
> @@ -16,7 +16,7 @@
> |ia64: | TODO |
> |m68k: | TODO |
> | microblaze: | ok |
> -|mips: | TODO |
> +|mips: | ok |
> | nds32: | TODO |
> | nios2: | TODO |
> |openrisc: | TODO |
> diff --git a/Documentation/features/debug/kmemleak/arch-support.txt
> b/Documentation/features/debug/kmemleak/arch-support.txt
> index 915185634611..e9ac415f8aec 100644
> --- a/Documentation/features/debug/kmemleak/arch-support.txt
> +++ b/Documentation/features/debug/kmemleak/arch-support.txt
> @@ -10,7 +10,7 @@
> | arc: | ok |
> | arm: | ok |
> | arm64: | ok |
> -|csky: | TODO |
> +|csky: | ok |
> | h8300: | TODO |
> | hexagon: | TODO |
> |ia64: | TODO |
> diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> index aade7816cb87..96156e8802a7 100644
> --- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> +++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
> @@ -22,7 +22,7 @@
> |openrisc: | TODO |
> | parisc: | ok |
> | powerpc: | ok |
> -| riscv: | TODO |
> +| riscv: | ok |
> |s390: | ok |
> | sh: | TODO |
> | sparc: | TODO |
> diff --git a/Documentation/features/debug/kprobes/arch-support.txt
> b/Documentation/features/debug/kprobes/arch-support.txt
> index 4b663c124d1c..ee95ed61909a 100644
> --- a/Documentation/features/debug/kprobes/arch-support.txt
> +++ b/Documentation/features/debug/kprobes/arch-support.txt
> @@ -22,7 +22,7 @@
> |openrisc: | TODO |
> | parisc: | ok |
> | powerpc: | ok |
> -| riscv: | TODO |
> +| riscv: | ok |
> |s390: | ok |
> | sh: | ok |
> | sparc: | ok |
> diff --git a/Documentation/features/debug/kretprobes/arch-support.txt
> b/Documentation/features/debug/kretprobes/arch-support.txt
> index 5449bb808442..612cb97d47b8 100644
> --- a/Documentation/features/debug/kretprobes/arch-support.txt
> +++ b/Documentation/features/debug/kretprobes/arch-support.txt
> @@ -22,7 +22,7 @@
> |openrisc: | TODO |
> | parisc: | ok |
> | powerpc: | ok |
> -| riscv: | TODO |
> +| riscv: | ok |
> |s390: | ok |
> | sh: | ok |
> | sparc: | ok |
> diff --git a/Documentation/features/debug/uprobes/arch-support.txt
> b/Documentation/features/debug/uprobes/arch-support.txt
> index 2820177787e1..8
Re: [PATCH v2 2/2] drivers/clocksource: Fixup csky,mptimer compile error with CPU_CK610
Hi Daniel, On Wed, Feb 10, 2021 at 4:26 AM Daniel Lezcano wrote: > > On 09/02/2021 17:02, Guo Ren wrote: > > Hi Daniel, > > > > On Sun, Feb 7, 2021 at 5:29 PM Daniel Lezcano > > wrote: > >> > >> On 07/02/2021 04:31, Guo Ren wrote: > >>> Hi Daniel, > >>> > >>> On Thu, Feb 4, 2021 at 4:48 PM Daniel Lezcano > >>> wrote: > >>>> > >>>> On 04/02/2021 08:46, guo...@kernel.org wrote: > >>>>> From: Guo Ren > >>>>> > >>>>> The timer-mp-csky.c only could support CPU_CK860 and it will > >>>>> compile error with CPU_CK610. > >>>>> > >>>>> It has been selected in arch/csky/Kconfig. > >>>> > >>>> It would be better if you fix the root cause of the compilation error. > >>> The timer-mp-csky.c has used specific instructions which only > >>> supported by CK860 and timer-mp-csky.c is only design for CK860. > >> > >> I guess you are referring to mfcr() ? > >> > >>> In arch/csky/Konfig we only select it with CK860. > >>> select CSKY_MPINTC if CPU_CK860 > >>> select CSKY_MP_TIMER if CPU_CK860 > >>> > >>> So here let's select timer-mp-csky.c in arch/csky/Kconfig, not in > >>> drivers/clocksource/Kconfig. > >> > >> The COMPILE_TEST option is there to let other architecture to compile > >> drivers and increase the compilation test coverage. > >> > >> The proposed change just removes the driver from this coverage. > > When we compile the csky arch with C860, it will be selected. > > > >> > >> Ideally, it would be better to keep it with the COMPILE_TEST option, so > >> changes impacting all the drivers can be caught before submitting the > >> patches. > >> > >> By just adding > >> > >> #ifndef mfcr > >> #define mfcr(a) 0 > >> #endif > > > > 610 couldn't support CSKY_MP_TIMER and it's only for 860. So it's not > > a coding skill issue. > > I think there is a misunderstanding. > > When I want to compile on x64 all the timer drivers, I do enable > COMPILE_TEST, then the strings appear and the drivers can be selected. > > If the COMPILE_TEST is not enabled, the string does not appear, it is > not possible to enable/disable it and the platform must enable it from > the aforementioned arch/csky/Konfig. > > Actually, the timer drivers policy is : drivers can not be enabled from > the drivers/clocksource/Kconfig, it is up to the platform Kconfig to > select them. The exception is when the COMPILE_TEST option is set for > testing purpose. > > The timer must compile on any other archs and the stubs for the platform > specific calls must be provided. > > Did I miss something with your changes ? I think our biggest difference is: - You think that CSKY_MPTIMER should not be related to the architecture, but can be compiled with any architecture. - But I think CSKY_MPTIMER only could to be compiled with CSKY C860. But from the perspective of easy maintenance, I agree with your suggestion. I will adopt in next patch: > >> #ifndef mfcr > >> #define mfcr(a) 0 > >> #endif Thx -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v2 2/2] drivers/clocksource: Fixup csky,mptimer compile error with CPU_CK610
Hi Daniel, On Sun, Feb 7, 2021 at 5:29 PM Daniel Lezcano wrote: > > On 07/02/2021 04:31, Guo Ren wrote: > > Hi Daniel, > > > > On Thu, Feb 4, 2021 at 4:48 PM Daniel Lezcano > > wrote: > >> > >> On 04/02/2021 08:46, guo...@kernel.org wrote: > >>> From: Guo Ren > >>> > >>> The timer-mp-csky.c only could support CPU_CK860 and it will > >>> compile error with CPU_CK610. > >>> > >>> It has been selected in arch/csky/Kconfig. > >> > >> It would be better if you fix the root cause of the compilation error. > > The timer-mp-csky.c has used specific instructions which only > > supported by CK860 and timer-mp-csky.c is only design for CK860. > > I guess you are referring to mfcr() ? > > > In arch/csky/Konfig we only select it with CK860. > > select CSKY_MPINTC if CPU_CK860 > > select CSKY_MP_TIMER if CPU_CK860 > > > > So here let's select timer-mp-csky.c in arch/csky/Kconfig, not in > > drivers/clocksource/Kconfig. > > The COMPILE_TEST option is there to let other architecture to compile > drivers and increase the compilation test coverage. > > The proposed change just removes the driver from this coverage. When we compile the csky arch with C860, it will be selected. > > Ideally, it would be better to keep it with the COMPILE_TEST option, so > changes impacting all the drivers can be caught before submitting the > patches. > > By just adding > > #ifndef mfcr > #define mfcr(a) 0 > #endif 610 couldn't support CSKY_MP_TIMER and it's only for 860. So it's not a coding skill issue. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] irqchip/csky: Use true and false for bool variable
Looks good to me, thx.
On Tue, Feb 9, 2021 at 6:08 PM Jiapeng Chong
wrote:
>
> Fix the following coccicheck warning:
>
> ./drivers/irqchip/irq-csky-apb-intc.c:139:9-10: WARNING: return of 0/1
> in function 'handle_irq_perbit' with return type bool.
>
> Reported-by: Abaci Robot
> Signed-off-by: Jiapeng Chong
> ---
> drivers/irqchip/irq-csky-apb-intc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-csky-apb-intc.c
> b/drivers/irqchip/irq-csky-apb-intc.c
> index 5a2ec43..5e18ba9 100644
> --- a/drivers/irqchip/irq-csky-apb-intc.c
> +++ b/drivers/irqchip/irq-csky-apb-intc.c
> @@ -136,11 +136,11 @@ static inline bool handle_irq_perbit(struct pt_regs
> *regs, u32 hwirq,
> u32 irq_base)
> {
> if (hwirq == 0)
> - return 0;
> + return false;
>
> handle_domain_irq(root_domain, irq_base + __fls(hwirq), regs);
>
> - return 1;
> + return true;
> }
>
> /* gx6605s 64 irqs interrupt controller */
> --
> 1.8.3.1
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v2 2/2] drivers/clocksource: Fixup csky,mptimer compile error with CPU_CK610
The same way in IRQ chip: Link: https://lore.kernel.org/lkml/161245410060.1472442.13488057609914274236.b4...@kernel.org/ On Sun, Feb 7, 2021 at 11:31 AM Guo Ren wrote: > > Hi Daniel, > > On Thu, Feb 4, 2021 at 4:48 PM Daniel Lezcano > wrote: > > > > On 04/02/2021 08:46, guo...@kernel.org wrote: > > > From: Guo Ren > > > > > > The timer-mp-csky.c only could support CPU_CK860 and it will > > > compile error with CPU_CK610. > > > > > > It has been selected in arch/csky/Kconfig. > > > > It would be better if you fix the root cause of the compilation error. > The timer-mp-csky.c has used specific instructions which only > supported by CK860 and timer-mp-csky.c is only design for CK860. > > In arch/csky/Konfig we only select it with CK860. > select CSKY_MPINTC if CPU_CK860 > select CSKY_MP_TIMER if CPU_CK860 > > So here let's select timer-mp-csky.c in arch/csky/Kconfig, not in > drivers/clocksource/Kconfig. > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v2 2/2] drivers/clocksource: Fixup csky,mptimer compile error with CPU_CK610
Hi Daniel, On Thu, Feb 4, 2021 at 4:48 PM Daniel Lezcano wrote: > > On 04/02/2021 08:46, guo...@kernel.org wrote: > > From: Guo Ren > > > > The timer-mp-csky.c only could support CPU_CK860 and it will > > compile error with CPU_CK610. > > > > It has been selected in arch/csky/Kconfig. > > It would be better if you fix the root cause of the compilation error. The timer-mp-csky.c has used specific instructions which only supported by CK860 and timer-mp-csky.c is only design for CK860. In arch/csky/Konfig we only select it with CK860. select CSKY_MPINTC if CPU_CK860 select CSKY_MP_TIMER if CPU_CK860 So here let's select timer-mp-csky.c in arch/csky/Kconfig, not in drivers/clocksource/Kconfig. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
[irqchip: irq/irqchip-next] irqchip/csky-mpintc: Prevent selection on unsupported platforms
The following commit has been merged into the irq/irqchip-next branch of irqchip: Commit-ID: be1abc5ba4d2082df6749ab95ec6f87c4d3dbb23 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/be1abc5ba4d2082df6749ab95ec6f87c4d3dbb23 Author:Guo Ren AuthorDate:Thu, 04 Feb 2021 15:46:08 +08:00 Committer: Marc Zyngier CommitterDate: Thu, 04 Feb 2021 10:37:28 irqchip/csky-mpintc: Prevent selection on unsupported platforms The irq-csky-mpintc driver is only supported on CPU_CK860 and it will generate a compilation error when selected with CPU_CK610. As it is already selected directly in the architecture Kconfig, drop the option to select it manually. Signed-off-by: Guo Ren [maz: rewrote commit message] Signed-off-by: Marc Zyngier Link: https://lore.kernel.org/r/20210204074609.3553018-1-guo...@kernel.org --- drivers/irqchip/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index f95d114..030895c 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -427,7 +427,7 @@ config QCOM_PDC IRQs for Qualcomm Technologies Inc (QTI) mobile chips. config CSKY_MPINTC - bool "C-SKY Multi Processor Interrupt Controller" + bool depends on CSKY help Say yes here to enable C-SKY SMP interrupt controller driver used
Re: [PATCH 1/2] drivers/irqchip: Fixup csky,mpintc compile error with CPU_CK610
Thx Marc, On Wed, Feb 3, 2021 at 11:44 PM Marc Zyngier wrote: > > On 2021-02-03 13:48, guo...@kernel.org wrote: > > From: Guo Ren > > > > The irq-csky-mpintc.c only could support CPU_CK860 and it will > > compile error with CPU_CK610. > > > > Signed-off-by: Guo Ren > > Cc: Marc Zyngier > > --- > > drivers/irqchip/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index b147f22a78f4..9be2dd5c6380 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -433,7 +433,7 @@ config QCOM_PDC > > > > config CSKY_MPINTC > > bool "C-SKY Multi Processor Interrupt Controller" > > - depends on CSKY > > + depends on CSKY && CPU_CK860 > > help > > Say yes here to enable C-SKY SMP interrupt controller driver used > > for C-SKY SMP system. > > I'm not convinced this is the right fix. > > You already select CSKY_MPINTC only when CPU_CK860 is selected, > so preventing the user from selecting it should simply be a matter > of dropping the string after bool. Yes, you are right. I will change it into arch/csky. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: change a Kconfig symbol name to fix e1000 build error
Acked-by Thx On Sun, Jan 31, 2021 at 7:50 AM Randy Dunlap wrote: > > e1000's #define of CONFIG_RAM_BASE conflicts with a Kconfig symbol in > arch/csky/Kconfig. > The symbol in e1000 has been around longer, so change arch/csky/ > to use DRAM_BASE instead of RAM_BASE to remove the conflict. > (although e1000 is also a 2-line change) > > Not tested: I don't have a build toolchain for CSKY. > > Signed-off-by: Randy Dunlap > Reported-by: kernel test robot > Cc: Jesse Brandeburg > Cc: Tony Nguyen > Cc: intel-wired-...@lists.osuosl.org > Cc: Guo Ren > Cc: Guo Ren > Cc: linux-c...@vger.kernel.org > --- > IMO "CONFIG_" namespace should belong to Kconfig files, not > individual drivers, but e1000 isn't the only driver that uses > CONFIG_ symbols. > > arch/csky/Kconfig|2 +- > arch/csky/include/asm/page.h |2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > --- linux-next-20210129.orig/arch/csky/include/asm/page.h > +++ linux-next-20210129/arch/csky/include/asm/page.h > @@ -28,7 +28,7 @@ > #define SSEG_SIZE 0x2000 > #define LOWMEM_LIMIT (SSEG_SIZE * 2) > > -#define PHYS_OFFSET_OFFSET (CONFIG_RAM_BASE & (SSEG_SIZE - 1)) > +#define PHYS_OFFSET_OFFSET (CONFIG_DRAM_BASE & (SSEG_SIZE - 1)) > > #ifndef __ASSEMBLY__ > > --- linux-next-20210129.orig/arch/csky/Kconfig > +++ linux-next-20210129/arch/csky/Kconfig > @@ -314,7 +314,7 @@ config FORCE_MAX_ZONEORDER > int "Maximum zone order" > default "11" > > -config RAM_BASE > +config DRAM_BASE > hex "DRAM start addr (the same with memory-section in dts)" > default 0x0 > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v1] csky: use free_initmem_default() in free_initmem()
Hi David, > 在 2021年1月27日，下午4:49，David Hildenbrand 写道： > > On 27.01.21 08:52, Guo Ren wrote: >> Thx Mike, > > Thanks for triggering a build/test! I'd be curious if there is an easy way to > trigger this myself (I assume, fork csky buildroot on gitlab, reroute CSKY > kernel repo, adjust CSKY_LINUX_NEXT_VERSION); if so, it would be worth > documenting - thanks! > 1. You can just simple fork our project, and enable your gitlab share-runner. 2. Modify https://gitlab.com/c-sky/buildroot/-/blob/master/package/csky-arch/csky-arch.mk Line 28: CSKY_ARCH_SITE = $(call github,c-sky,csky-linux,$(CSKY_ARCH_VERSION)) to your own forked our kernel repo Line 49: CSKY_LINUX_NEXT_VERSION= f0bbf052ae463b3802f1c7ba36b36da2f54945f2 change the version as you want You also need to remove some unused entries in .gitlab.yml. > Thanks Mike for the valuable links :) For now I was only relying on > cross-compilers as shipped by Fedora, now I can upgrade my cross-build > environment :) > >> It's under test: >> https://gitlab.com/c-sky/buildroot/-/pipelines/247353584 >> kernel: >> https://github.com/c-sky/csky-linux/commit/9d986b01feb991ded3fb8c1f8153a0c80ea84b9c > > > -- > Thanks, > > David / dhildenb Best Regards Guo Ren
Re: [PATCH v1] csky: use free_initmem_default() in free_initmem()
Thx Mike,
It's under test:
https://gitlab.com/c-sky/buildroot/-/pipelines/247353584
kernel:
https://github.com/c-sky/csky-linux/commit/9d986b01feb991ded3fb8c1f8153a0c80ea84b9c
On Wed, Jan 27, 2021 at 2:26 AM Mike Rapoport wrote:
>
> On Tue, Jan 26, 2021 at 07:14:20PM +0100, David Hildenbrand wrote:
> > The existing code is essentially
> > free_initmem_default()->free_reserved_area() without poisoning.
> >
> > Note that existing code missed to update the managed page count of the
> > zone.
> >
> > Cc: Andrew Morton
> > Cc: Guo Ren
> > Cc: Mike Rapoport
> > Cc: Oscar Salvador
> > Cc: Michal Hocko
> > Cc: Wei Yang
> > Signed-off-by: David Hildenbrand
> > ---
> >
> > Not compile tested as documentation on how to get
> > https://gitlab.com/c-sky/buildroot
> > running, especially with a custom kernel, is a bit sparse.
>
> You can pick a cross-compiler from here:
>
> https://mirrors.edge.kernel.org/pub/tools/crosstool/
>
> > ---
> > arch/csky/mm/init.c | 17 +
> > 1 file changed, 1 insertion(+), 16 deletions(-)
> >
> > diff --git a/arch/csky/mm/init.c b/arch/csky/mm/init.c
> > index 81e4e5e78f38..894050a8ce09 100644
> > --- a/arch/csky/mm/init.c
> > +++ b/arch/csky/mm/init.c
> > @@ -110,24 +110,9 @@ void __init mem_init(void)
> > mem_init_print_info(NULL);
> > }
> >
> > -extern char __init_begin[], __init_end[];
> > -
> > void free_initmem(void)
> > {
> > - unsigned long addr;
> > -
> > - addr = (unsigned long) &__init_begin;
> > -
> > - while (addr < (unsigned long) &__init_end) {
> > - ClearPageReserved(virt_to_page(addr));
> > - init_page_count(virt_to_page(addr));
> > - free_page(addr);
> > - totalram_pages_inc();
> > - addr += PAGE_SIZE;
> > - }
> > -
> > - pr_info("Freeing unused kernel memory: %dk freed\n",
> > - ((unsigned int)&__init_end - (unsigned int)&__init_begin) >> 10);
> > + free_initmem_default(-1);
> > }
> >
> > void pgd_init(unsigned long *p)
> > --
> > 2.29.2
> >
>
> --
> Sincerely yours,
> Mike.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] RISC-V: probes: Treat the instruction stream as host-endian
Acked-by: Guo Ren
On 2021/1/23 上午11:34, Palmer Dabbelt wrote:
From: Palmer Dabbelt
Neither of these are actually correct: the instruction stream is defined
(for versions of the ISA manual newer than 2.2) as a stream of 16-bit
little-endian parcels, which is different than just being little-endian.
In theory we should represent this as a type, but we don't have any
concrete plans for the big endian stuff so it doesn't seem worth the
time -- we've got variants of this all over the place.
Instead I'm just dropping the unnecessary type conversion, which is a
NOP on LE systems but causes an sparse error as the types are all mixed
up.
Reported-by: kernel test robot
Signed-off-by: Palmer Dabbelt
---
arch/riscv/kernel/probes/decode-insn.c | 2 +-
arch/riscv/kernel/probes/kprobes.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/probes/decode-insn.c
b/arch/riscv/kernel/probes/decode-insn.c
index 0876c304ca77..0ed043acc882 100644
--- a/arch/riscv/kernel/probes/decode-insn.c
+++ b/arch/riscv/kernel/probes/decode-insn.c
@@ -16,7 +16,7 @@
enum probe_insn __kprobes
riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)
{
- probe_opcode_t insn = le32_to_cpu(*addr);
+ probe_opcode_t insn = *addr;
/*
* Reject instructions list:
diff --git a/arch/riscv/kernel/probes/kprobes.c
b/arch/riscv/kernel/probes/kprobes.c
index e60893bd87db..a2ec18662fee 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -57,7 +57,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
}
/* copy instruction */
- p->opcode = le32_to_cpu(*p->addr);
+ p->opcode = *p->addr;
/* decode instruction */
switch (riscv_probe_decode_insn(p->addr, >ainsn.api)) {
Re: [PATCH v2 1/5] csky: Remove custom asm/atomic.h implementation
Hi Peter, On Thu, Jan 7, 2021 at 7:19 PM Peter Zijlstra wrote: > > On Sun, Dec 20, 2020 at 03:39:19PM +, guo...@kernel.org wrote: > > From: Guo Ren > > > > Use generic atomic implementation based on cmpxchg. So remove csky > > asm/atomic.h. > > Clarification would be good. Typically cmpxchg() loops perform > sub-optimal on LL/SC architectures, due to the double loop construction. Yes, you are right. But I still want to use comm cmpxchg instead of my implementation. Maybe in the future, we'll optimize it back. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v2 4/5] csky: Fixup asm/cmpxchg.h with correct ordering barrier
Hi Peter,
On Thu, Jan 7, 2021 at 8:41 PM Peter Zijlstra wrote:
>
> On Sun, Dec 20, 2020 at 03:39:22PM +, guo...@kernel.org wrote:
>
>
> > +#define cmpxchg(ptr, o, n) \
> > +({ \
> > + __typeof__(*(ptr)) __ret; \
> > + __smp_release_fence(); \
> > + __ret = cmpxchg_relaxed(ptr, o, n); \
> > + __smp_acquire_fence(); \
> > + __ret; \
> > +})
>
> So you failed to Cc me on patch #2 that introduces these barriers. I've
> dug it out, but I'm still terribly confused on all that.
>
> On first reading the above looks wrong.
>
> Could you also clarify the difference (if any) between your bar.brwarw
> and sync instruction?
>
> Specifically, about transitiviry, or whatever we seem to be calling that
> today.
bar.brwarw just like riscv fence.rwrw
bar means barrier
brw means before read and write would happen before the instruction.
arw means after read and write would happen after the instruction
So it also could be bar.brarw / bar.arw / bar.brw / bar.braw
sync means we need to wait until all instructions complete in the CPU
pipeline and then issue the next instructions.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] riscv: Remove duplicate definition in pagtable.h
On Tue, Jan 12, 2021 at 4:18 PM Atish Patra wrote: > > On Mon, Jan 11, 2021 at 6:38 PM wrote: > > > > From: Guo Ren > > > > PAGE_KERNEL_EXEC has been defined above. > > > > Signed-off-by: Guo Ren > > Cc: Palmer Dabbelt > > Cc: Pekka Enberg > > --- > > arch/riscv/include/asm/pgtable.h | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/pgtable.h > > b/arch/riscv/include/asm/pgtable.h > > index 41a72861987c..dff51a84e52e 100644 > > --- a/arch/riscv/include/asm/pgtable.h > > +++ b/arch/riscv/include/asm/pgtable.h > > @@ -101,7 +101,6 @@ > > #define PAGE_KERNEL__pgprot(_PAGE_KERNEL) > > #define PAGE_KERNEL_EXEC __pgprot(_PAGE_KERNEL | _PAGE_EXEC) > > #define PAGE_KERNEL_READ __pgprot(_PAGE_KERNEL & ~_PAGE_WRITE) > > -#define PAGE_KERNEL_EXEC __pgprot(_PAGE_KERNEL | _PAGE_EXEC) > > #define PAGE_KERNEL_READ_EXEC __pgprot((_PAGE_KERNEL & ~_PAGE_WRITE) \ > > | _PAGE_EXEC) > > > > -- > > 2.17.1 > > > > > > ___ > > linux-riscv mailing list > > linux-ri...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv > > It's already on the fixes. > > http://lists.infradead.org/pipermail/linux-riscv/2021-January/004134.html Great, ignore mine :) -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: kprobe: fix unreachable code in simulate_blz32
Hi menglong,
On Mon, Jan 11, 2021 at 10:25 PM wrote:
>
> From: Menglong Dong
>
> The type of 'val' is 'unsigned long' in simulate_blz32, so 'val < 0'
> can't be true.
>
> When 'csky_insn_reg_get_val' fails, 'false' will be returned. We
> can directly use its return value here.
>
> Fixes: 33e53ae1ce41 ("csky: Add kprobes supported")
> Signed-off-by: Menglong Dong
> ---
> arch/csky/kernel/probes/simulate-insn.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/csky/kernel/probes/simulate-insn.c
> b/arch/csky/kernel/probes/simulate-insn.c
> index 4e464fed52ec..b09ddcbcfa18 100644
> --- a/arch/csky/kernel/probes/simulate-insn.c
> +++ b/arch/csky/kernel/probes/simulate-insn.c
> @@ -346,9 +346,7 @@ simulate_blz32(u32 opcode, long addr, struct pt_regs
> *regs)
> unsigned long tmp = opcode & 0x1f;
> unsigned long val;
>
> - csky_insn_reg_get_val(regs, tmp, );
> -
> - if (val < 0) {
if ((long) val < 0) {
?
We need val to determine branch taken or not.
> + if (!csky_insn_reg_get_val(regs, tmp, )) {
> instruction_pointer_set(regs,
> addr + sign_extend32((opcode & 0x) >> 15,
> 15));
> } else
> --
> 2.17.1
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH/v2] csky: Fix typos in Kconfig
Thx, looks good to me. On Thu, Jan 7, 2021 at 12:23 AM Randy Dunlap wrote: > > On 1/6/21 2:04 AM, Masanari Iida wrote: > > This patch fixes some spelling typos in Kconfig. > > > > Signed-off-by: Masanari Iida > > --- > > arch/csky/Kconfig | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig > > index 7f1721101ea0..e6ddca10e3ee 100644 > > --- a/arch/csky/Kconfig > > +++ b/arch/csky/Kconfig > > @@ -243,9 +243,9 @@ menuconfig HAVE_TCM > > bool "Tightly-Coupled/Sram Memory" > > select GENERIC_ALLOCATOR > > help > > - The implementation are not only used by TCM (Tightly-Coupled Meory) > > - but also used by sram on SOC bus. It follow existed linux tcm > > - software interface, so that old tcm application codes could be > > + The implementation is not only used by TCM (Tightly-Coupled Memory) > > + but also used by SRAM on SOC bus. It follows the existing Linux TCM > > + software interface, so that old TCM application codes could be > > re-used directly. > > > > if HAVE_TCM > > > > LGTM. Thanks. > Reviewed-by: Randy Dunlap > > -- > ~Randy -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] csky: Fix a size determination in gpr_get()
Hi Zhengzhong, I'll take it, thx. On Wed, Dec 23, 2020 at 10:31 AM Zhenzhong Duan wrote: > > On Wed, Sep 23, 2020 at 12:52 PM Al Viro wrote: > > > > On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote: > > > > > > What's going on there? The mapping is really weird - assuming > > > > you had v0..v31 in the first 32 elements of regs->vr[], you > > > > end up with > > > > > > > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15 > > > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31 > > > > > > > > in the beginning of the output. Assuming it is the intended > > > > behaviour, it's probably worth some comments... > > > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP > > > regs' width is 128b. > > > > > > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0] > > > ... > > > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15] > > > vr[64], vr[65] = fp[16] > > > vr[66], vr[67] = fp[17] > > > ... > > > vr[94], vr[95] = fp[31] > > > > > > Yeah, this is confusing and I'll add a comment later. > > > > Umm... It would help if you described these 3 layouts: > > 1) kernel-side with VDSP > > 2) userland (identical to (1)?) > > 3) kernel-side without VDSP > > Still confused... > > > > PS: my apologies re commit message - I left a note to myself when doing > > that series and then forgot about it ;-/ > > > > Anyway, which tree should it go through? In any case, that fix is > > Acked-by: Al Viro > > and I can take it through vfs.git or you guys can pick in csky tree; > > up to you. > > Hi Al, Guo > > Seems this patch is still pending， could you help check it？Thanks > > Zhenzhong -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/2] futex: mark futex_detect_cmpxchg() as 'noinline'
Hi Arnd, On Mon, Dec 21, 2020 at 1:49 AM Arnd Bergmann wrote: > > On Sun, Dec 20, 2020 at 4:46 PM Guo Ren wrote: > > On Tue, Dec 15, 2020 at 7:26 PM Arnd Bergmann wrote: > > > > > > On Tue, Dec 15, 2020 at 7:09 AM Guo Ren wrote: > > > > On Mon, Dec 14, 2020 at 9:15 PM Arnd Bergmann wrote: > > > > > I had a look at what other architectures always implement > > > > > futex_atomic_cmpxchg_inatomic() or can use the asm-generic non-SMP > > > > > version, > > > > > and I found that it's pretty much all of them, the odd ones being > > > > > just sparc32 > > > > > and csky, which use asm-generic/futex.h but do have an SMP option, > > > > > as well as xtensa > > > > > > > > > > I would guess that for csky, this is a mistake, as the architecture > > > > > is fairly > > > > > new and should be able to implement it. Not sure about sparc32. > > > > > > > > The c610, c807, c810 don't support SMP, so futex_cmpxchg_enabled = 1 > > > > with asm-generic's implementation. > > > > For c860, there is no HAVE_FUTEX_CMPXCHG and cmpxchg_inatomic/inuser > > > > implementation, so futex_cmpxchg_enabled = 0. > > > > > > > > Thx for point it out, we'll implement cmpxchg_inatomic/inuser for C860 > > > > and still use asm-generic for non-smp CPUs. > > > > > > Sounds good to me. > > Done: > > https://lore.kernel.org/linux-csky/1608478763-60148-3-git-send-email-guo...@kernel.org/T/#u > > Thanks! > > Can you clarify if there are any dependencies on the other patches in > that series? No dependency. > > I'd like to take the futex patch through the asm-generic tree along with the > patches for the other architectures. You take the futex patch and I'll remove it from my tree. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/2] futex: mark futex_detect_cmpxchg() as 'noinline'
Hi Arnd, On Tue, Dec 15, 2020 at 7:26 PM Arnd Bergmann wrote: > > On Tue, Dec 15, 2020 at 7:09 AM Guo Ren wrote: > > On Mon, Dec 14, 2020 at 9:15 PM Arnd Bergmann wrote: > > > I had a look at what other architectures always implement > > > futex_atomic_cmpxchg_inatomic() or can use the asm-generic non-SMP > > > version, > > > and I found that it's pretty much all of them, the odd ones being just > > > sparc32 > > > and csky, which use asm-generic/futex.h but do have an SMP option, > > > as well as xtensa > > > > > > I would guess that for csky, this is a mistake, as the architecture is > > > fairly > > > new and should be able to implement it. Not sure about sparc32. > > > > The c610, c807, c810 don't support SMP, so futex_cmpxchg_enabled = 1 > > with asm-generic's implementation. > > For c860, there is no HAVE_FUTEX_CMPXCHG and cmpxchg_inatomic/inuser > > implementation, so futex_cmpxchg_enabled = 0. > > > > Thx for point it out, we'll implement cmpxchg_inatomic/inuser for C860 > > and still use asm-generic for non-smp CPUs. > > Sounds good to me. Done: https://lore.kernel.org/linux-csky/1608478763-60148-3-git-send-email-guo...@kernel.org/T/#u -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/2] futex: mark futex_detect_cmpxchg() as 'noinline'
Hi Arnd,
On Mon, Dec 14, 2020 at 9:15 PM Arnd Bergmann wrote:
>
> On Sat, Dec 12, 2020 at 9:01 PM Thomas Gleixner wrote:
> >
> > On Sat, Dec 12 2020 at 13:26, Marco Elver wrote:
> > > On Thu, Mar 07, 2019 at 10:14AM +0100, Arnd Bergmann wrote:
> > >> -static void __init futex_detect_cmpxchg(void)
> > >> +static noinline void futex_detect_cmpxchg(void)
> > >> {
> > >> #ifndef CONFIG_HAVE_FUTEX_CMPXCHG
> > >> u32 curval;
> > >
> > > What ever happened to this patch?
> >
> > It obviously fell through the cracks.
> >
> > > I'm seeing this again with the attached config + next-20201211 (for
> > > testing https://bugs.llvm.org/show_bug.cgi?id=48492). Had to apply this
> > > patch to build the kernel.
> >
> > What really bothers me is to remove the __init from a function which is
> > clearly only used during init. And looking deeper it's simply a hack.
> >
> > This function is only needed when an architecture has to runtime
> > discover whether the CPU supports it or not. ARM has unconditional
> > support for this, so the obvious thing to do is the below.
> >
>
> Ah perfect, that is clearly the right solution here.
>
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -86,6 +86,7 @@ config ARM
> > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG
> > select HAVE_FUNCTION_TRACER if !XIP_KERNEL
> > + select HAVE_FUTEX_CMPXCHG if FUTEX
> > select HAVE_GCC_PLUGINS
> > select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K ||
> > CPU_V7)
> > select HAVE_IDE if PCI || ISA || PCMCIA
>
> I had a look at what other architectures always implement
> futex_atomic_cmpxchg_inatomic() or can use the asm-generic non-SMP version,
> and I found that it's pretty much all of them, the odd ones being just sparc32
> and csky, which use asm-generic/futex.h but do have an SMP option,
> as well as xtensa
>
> I would guess that for csky, this is a mistake, as the architecture is fairly
> new and should be able to implement it. Not sure about sparc32.
The c610, c807, c810 don't support SMP, so futex_cmpxchg_enabled = 1
with asm-generic's implementation.
For c860, there is no HAVE_FUTEX_CMPXCHG and cmpxchg_inatomic/inuser
implementation, so futex_cmpxchg_enabled = 0.
Thx for point it out, we'll implement cmpxchg_inatomic/inuser for C860
and still use asm-generic for non-smp CPUs:
diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig
index a2189c0..e968c58 100644
--- a/arch/csky/Kconfig
+++ b/arch/csky/Kconfig
@@ -49,6 +49,7 @@ config CSKY
select HAVE_FUNCTION_TRACER
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_ERROR_INJECTION
+ select HAVE_FUTEX_CMPXCHG if FUTEX && SMP
select HAVE_FTRACE_MCOUNT_RECORD
select HAVE_KERNEL_GZIP
select HAVE_KERNEL_LZO
diff --git a/arch/csky/include/asm/futex.h b/arch/csky/include/asm/futex.h
new file mode 100644
index ..29275e8
--- /dev/null
+++ b/arch/csky/include/asm/futex.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_CSKY_FUTEX_H
+#define __ASM_CSKY_FUTEX_H
+
+#ifndef CONFIG_SMP
+#include
+#else
+#include
+#include
+#include
+
+static inline int
+arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
+{
+ int oldval = 0, ret = 0;
+
+ if (!access_ok(uaddr, sizeof(u32)))
+ return -EFAULT;
+
+ <...>
+
+ return ret;
+}
+
+static inline int
+futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
+ u32 oldval, u32 newval)
+{
+ int ret = 0;
+ u32 val;
+ uintptr_t tmp;
+
+ if (!access_ok(uaddr, sizeof(u32)))
+ return -EFAULT;
+
+ <...>
+
+ return ret;
+}
+#endif
+#endif /* __ASM_CSKY_FUTEX_H */
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 2/5] riscv: Add QUEUED_SPINLOCKS & QUEUED_RWLOCKS supported
Hi Will,
On Wed, Nov 25, 2020 at 10:31 PM Will Deacon wrote:
>
> On Wed, Nov 25, 2020 at 03:16:45PM +0100, Peter Zijlstra wrote:
> > @@ -207,6 +187,32 @@ static __always_inline void
> > clear_pending_set_locked(struct qspinlock *lock)
> > atomic_add(-_Q_PENDING_VAL + _Q_LOCKED_VAL, >val);
> > }
> >
> > +#endif /* _Q_PENDING_BITS == 8 */
> > +
> > +#if _Q_PENDING_BITS == 8 && ARCH_HAS_XCHG16
> > +
> > +/*
> > + * xchg_tail - Put in the new queue tail code word & retrieve previous one
> > + * @lock : Pointer to queued spinlock structure
> > + * @tail : The new queue tail code word
> > + * Return: The previous queue tail code word
> > + *
> > + * xchg(lock, tail), which heads an address dependency
> > + *
> > + * p,*,* -> n,*,* ; prev = xchg(lock, node)
> > + */
> > +static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> > +{
> > + /*
> > + * We can use relaxed semantics since the caller ensures that the
> > + * MCS node is properly initialized before updating the tail.
> > + */
> > + return (u32)xchg_relaxed(>tail,
> > + tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
> > +}
> > +
> > +#else /* !(_Q_PENDING_BITS == 8 && ARCH_HAS_XCHG16) */
>
> Why can't architectures just implement this with a 32-bit xchg instruction
> if they don't have one that operates on 16 bits? Sure, they'll store more
> data, but it's atomic so you shouldn't be able to tell... (ignoring parisc
> crazy).
>
> Also, I'm surprised qspinlock benefits riscv. On arm64, there's nothing in
> it over tickets for <= 16 CPUs.
NUMA is on the way:
https://lore.kernel.org/linux-riscv/20201119003829.1282810-1-atish.pa...@wdc.com/
With your advice, I think we could using tickets lock when <= 16 CPUs
and using qspinlock when > 16 CPUs.
Is that right?
The next patchset plan is:
- Using tickets & qspinlock together in riscv. Abandon 16bits
xchg/cmpxchg implementation.
- Abanden qspinlock in csky, because it only could 4 CPUs' SMP.
>
> Will
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/5] riscv: Coding convention for xchg
On Tue, Nov 24, 2020 at 10:29 PM Peter Zijlstra wrote:
>
> On Tue, Nov 24, 2020 at 01:43:53PM +, guo...@kernel.org wrote:
> > From: Guo Ren
> >
> > This is prepare for QUEUED_SPINLOCKS which need xchg support short
> > type value.
> > - Remove unused codes (xchg32, xchg64, cmpxchg32 ...)
> > - Combine xchg_relaxed, xchg_acquire, xchg_release into one asm
> > - Make atomic.aq/rl with seperated fence acquire & release
>
> Every time you find yourself doing multiple things, make it multiple
> patches.
Ok.
>
> > @@ -242,58 +239,58 @@ static __always_inline s64
> > atomic64_fetch_add_unless(atomic64_t *v, s64 a, s64 u
> > * atomic_{cmp,}xchg is required to have exactly the same ordering
> > semantics as
> > * {cmp,}xchg and the operations that return, so they need a full barrier.
> > */
> > +#define ATOMIC_OP(c_t, prefix)
> > \
> > static __always_inline
> > \
> > c_t atomic##prefix##_xchg_relaxed(atomic##prefix##_t *v, c_t n)
> > \
> > {\
> > + return xchg_relaxed(&(v->counter), n); \
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_xchg_acquire(atomic##prefix##_t *v, c_t n)
> > \
> > {\
> > + return xchg_acquire(&(v->counter), n); \
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_xchg_release(atomic##prefix##_t *v, c_t n)
> > \
> > {\
> > + return xchg_release(&(v->counter), n); \
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_xchg(atomic##prefix##_t *v, c_t n)
> > \
> > {\
> > + return xchg(&(v->counter), n); \
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_cmpxchg_relaxed(atomic##prefix##_t *v, \
> >c_t o, c_t n) \
> > {\
> > + return cmpxchg_relaxed(&(v->counter), o, n);\
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_cmpxchg_acquire(atomic##prefix##_t *v, \
> >c_t o, c_t n) \
> > {\
> > + return cmpxchg_acquire(&(v->counter), o, n);\
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_cmpxchg_release(atomic##prefix##_t *v, \
> >c_t o, c_t n) \
> > {\
> > + return cmpxchg_release(&(v->counter), o, n);\
> > }\
> > static __always_inline
> > \
> > c_t atomic##prefix##_cmpxchg(atomic##prefix##_t *v, c_t o, c_t n)\
> > {\
> > + return cmpxchg(&(v->counter), o, n);\
> > }
>
> > diff --git a/arch/riscv/include/asm/cmpxchg.h
> > b/arch/riscv/include/asm/cmpxchg.h
> > index 262e5bb..5609185 100644
> > --- a/arch/riscv/include/asm/cmpxchg.h
> > +++ b/arch/riscv
Re: [PATCH 2/5] riscv: Add QUEUED_SPINLOCKS & QUEUED_RWLOCKS supported
On Tue, Nov 24, 2020 at 11:00 PM Arnd Bergmann wrote:
>
> On Tue, Nov 24, 2020 at 3:39 PM Peter Zijlstra wrote:
> > On Tue, Nov 24, 2020 at 01:43:54PM +, guo...@kernel.org wrote:
> > > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
>
> > > + if (align) {\
> > > + __asm__ __volatile__ ( \
> > > + "0: lr.w %0, 0(%z4)\n" \
> > > + " move %1, %0\n" \
> > > + " slli %1, %1, 16\n" \
> > > + " srli %1, %1, 16\n" \
> > > + " move %2, %z3\n" \
> > > + " slli %2, %2, 16\n" \
> > > + " or %1, %2, %1\n" \
> > > + " sc.w %2, %1, 0(%z4)\n" \
> > > + " bnez %2, 0b\n" \
> > > + " srli %0, %0, 16\n" \
> > > + : "=" (__ret), "=" (tmp), "=" (__rc) \
> > > + : "rJ" (__new), "rJ"(addr) \
> > > + : "memory");\
> >
> > I'm pretty sure there's a handfull of implementations like this out
> > there... if only we could share.
>
> Isn't this effectively the same as the "_Q_PENDING_BITS != 8"
> version of xchg_tail()?
This can be concluded as the different effectiveness between cmpxchg
and xchg. For the arch which only has lr/sc instructions, the cmpxchg
& xchg are similar.
#if _Q_PENDING_BITS == 8
static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
{
/*
* We can use relaxed semantics since the caller ensures that the
* MCS node is properly initialized before updating the tail.
*/
return (u32)xchg_relaxed(>tail,
tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
}
#else /* _Q_PENDING_BITS == 8 */
static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
{
u32 old, new, val = atomic_read(>val);
for (;;) {
new = (val & _Q_LOCKED_PENDING_MASK) | tail;
/*
* We can use relaxed semantics since the caller ensures that
* the MCS node is properly initialized before updating the
* tail.
*/
old = atomic_cmpxchg_relaxed(>val, val, new);
if (old == val)
break;
val = old;
}
return old;
}
#endif /* _Q_PENDING_BITS == 8 */
>
> If nothing else needs xchg() on a 16-bit value, maybe
> changing the #ifdef in the qspinlock code is enough.
>
> Only around half the architectures actually implement 8-bit
> and 16-bit cmpxchg() and xchg(), it might even be worth trying
> to eventually change the interface to not do it at all, but
> instead have explicit cmpxchg8() and cmpxchg16() helpers
> for the few files that do use them.
>
> Arnd
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 2/5] riscv: Add QUEUED_SPINLOCKS & QUEUED_RWLOCKS supported
Thx Peter,
On Tue, Nov 24, 2020 at 10:39 PM Peter Zijlstra wrote:
>
> On Tue, Nov 24, 2020 at 01:43:54PM +, guo...@kernel.org wrote:
> > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> > index 59dd7be..6f5f438 100644
> > --- a/arch/riscv/include/asm/Kbuild
> > +++ b/arch/riscv/include/asm/Kbuild
> > @@ -6,3 +6,6 @@ generic-y += kvm_para.h
> > generic-y += local64.h
> > generic-y += user.h
> > generic-y += vmlinux.lds.h
> > +generic-y += mcs_spinlock.h
> > +generic-y += qrwlock.h
> > +generic-y += qspinlock.h
> > diff --git a/arch/riscv/include/asm/cmpxchg.h
> > b/arch/riscv/include/asm/cmpxchg.h
> > index 5609185..e178700 100644
> > --- a/arch/riscv/include/asm/cmpxchg.h
> > +++ b/arch/riscv/include/asm/cmpxchg.h
> > @@ -16,7 +16,43 @@
> > __typeof__(ptr) __ptr = (ptr); \
> > __typeof__(new) __new = (new); \
> > __typeof__(*(ptr)) __ret; \
> > + register unsigned long __rc, tmp, align, addr; \
> > switch (size) { \
> > + case 2: \
> > + align = ((unsigned long) __ptr & 0x3); \
> > + addr = ((unsigned long) __ptr & ~0x3); \
> > + if (align) {\
> > + __asm__ __volatile__ ( \
> > + "0: lr.w %0, 0(%z4)\n" \
> > + " move %1, %0\n" \
> > + " slli %1, %1, 16\n" \
> > + " srli %1, %1, 16\n" \
> > + " move %2, %z3\n" \
> > + " slli %2, %2, 16\n" \
> > + " or %1, %2, %1\n" \
> > + " sc.w %2, %1, 0(%z4)\n" \
> > + " bnez %2, 0b\n" \
> > + " srli %0, %0, 16\n" \
> > + : "=" (__ret), "=" (tmp), "=" (__rc) \
> > + : "rJ" (__new), "rJ"(addr) \
> > + : "memory");\
> > + } else {\
> > + __asm__ __volatile__ ( \
> > + "0: lr.w %0, (%z4)\n" \
> > + " move %1, %0\n" \
> > + " srli %1, %1, 16\n" \
> > + " slli %1, %1, 16\n" \
> > + " move %2, %z3\n" \
> > + " or %1, %2, %1\n" \
> > + " sc.w %2, %1, 0(%z4)\n" \
> > + " bnez %2, 0b\n" \
> > + " slli %0, %0, 16\n" \
> > + " srli %0, %0, 16\n" \
> > + : "=" (__ret), "=" (tmp), "=" (__rc) \
> > + : "rJ" (__new), "rJ"(addr) \
> > + : "memory");\
> > + } \
> > + break; \
> > case 4: \
> > __asm__ __volatile__ ( \
> > " amoswap.w %0, %2, %1\n" \
>
> I'm pretty sure there's a handfull of implementations like this out
> there... if only we could share.
Michael has sent qspinlock before, ref to Link below. He reused mips' code.
Link:
https://lore.kernel.org/linux-riscv/20190211043829.30096-1-michaeljcl...@mac.com/
Which short xchg implementation do you prefer (Mine or his)?
>
> Anyway, this too should be an independent patch.
Ok, I'll separate it into two patches,
1. implement short xchg
2. qspinlock enabled based on Michael's patch
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: set_thread_area.2: csky architecture undocumented
Thx Michael & Alejandro,
Yes, the man page has no csky's.
C-SKY have abiv1 and abiv2
For abiv1: There is no register for tls saving, We use trap 3 to got
tls and use set_thread_area to init ti->tp_value.
For abiv2: The r31 is the tls register. We could directly read r31 to
got r31 and use set_thread_area to init reg->tls value.
In glibc:
# ifdef __CSKYABIV2__
/* Define r31 as thread pointer register. */
# define READ_THREAD_POINTER() \
mov r0, r31;
# else
# define READ_THREAD_POINTER() \
trap 3;
# endif
/* Code to initially initialize the thread pointer. This might need
special attention since 'errno' is not yet available and if the
operation can cause a failure 'errno' must not be touched. */
# define TLS_INIT_TP(tcbp) \
({ INTERNAL_SYSCALL_DECL (err); \
long result_var; \
result_var = INTERNAL_SYSCALL (set_thread_area, err, 1,\
(char *) (tcbp) + TLS_TCB_OFFSET); \
INTERNAL_SYSCALL_ERROR_P (result_var, err) \
? "unknown error" : NULL; })
In kernel:
SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
{
struct thread_info *ti = task_thread_info(current);
struct pt_regs *reg = current_pt_regs();
reg->tls = addr;
ti->tp_value = addr;
return 0;
}
Any comments are welcome :)
On Tue, Nov 24, 2020 at 5:51 PM Michael Kerrisk (man-pages)
wrote:
>
> Hi Alex,
>
> On 11/23/20 10:31 PM, Alejandro Colomar (man-pages) wrote:
> > Hi Michael,
> >
> > SYNOPSIS
> >#include
> >
> >#if defined __i386__ || defined __x86_64__
> ># include
> >
> >int get_thread_area(struct user_desc *u_info);
> >int set_thread_area(struct user_desc *u_info);
> >
> >#elif defined __m68k__
> >
> >int get_thread_area(void);
> >int set_thread_area(unsigned long tp);
> >
> >#elif defined __mips__
> >
> >int set_thread_area(unsigned long addr);
> >
> >#endif
> >
> >Note: There are no glibc wrappers for these system calls; see
> >NOTES.
> >
> >
> > $ grep -rn 'SYSCALL_DEFINE.*et_thread_area'
> > arch/csky/kernel/syscall.c:6:
> > SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
> > arch/mips/kernel/syscall.c:86:
> > SYSCALL_DEFINE1(set_thread_area, unsigned long, addr)
> > arch/x86/kernel/tls.c:191:
> > SYSCALL_DEFINE1(set_thread_area, struct user_desc __user *, u_info)
> > arch/x86/kernel/tls.c:243:
> > SYSCALL_DEFINE1(get_thread_area, struct user_desc __user *, u_info)
> > arch/x86/um/tls_32.c:277:
> > SYSCALL_DEFINE1(set_thread_area, struct user_desc __user *, user_desc)
> > arch/x86/um/tls_32.c:325:
> > SYSCALL_DEFINE1(get_thread_area, struct user_desc __user *, user_desc)
> >
> >
> > See kernel commit 4859bfca11c7d63d55175bcd85a75d6cee4b7184
> >
> >
> > I'd change
> > - #elif defined __mips__
> > + #elif defined(__mips__ || __csky__)
> >
> > and then change the rest of the text to add csky when appropriate.
> > Am I correct?
>
> AFAICT, you are correct. I think the reason that csky is missing is
> that the architecture was added after this manual pages was added.
>
> Thanks,
>
> Michael
>
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4] riscv: Enable per-task stack canaries
Hi Palmer,
Could you help move the patch into your next-tree with Kees' review added?
On Sat, Nov 14, 2020 at 6:57 AM Kees Cook wrote:
>
> On Sun, Oct 18, 2020 at 12:38:17PM +, guo...@kernel.org wrote:
> > From: Guo Ren
> >
> > This enables the use of per-task stack canary values if GCC has
> > support for emitting the stack canary reference relative to the
> > value of tp, which holds the task struct pointer in the riscv
> > kernel.
> >
> > After compare arm64 and x86 implementations, seems arm64's is more
> > flexible and readable. The key point is how gcc get the offset of
> > stack_canary from gs/el0_sp.
> >
> > x86: Use a fix offset from gs, not flexible.
> >
> > struct fixed_percpu_data {
> > /*
> >* GCC hardcodes the stack canary as %gs:40. Since the
> >* irq_stack is the object at %gs:0, we reserve the bottom
> >* 48 bytes of the irq stack for the canary.
> >*/
> > chargs_base[40]; // :(
> > unsigned long stack_canary;
> > };
> >
> > arm64: Use -mstack-protector-guard-offset & guard-reg
> > gcc options:
> > -mstack-protector-guard=sysreg
> > -mstack-protector-guard-reg=sp_el0
> > -mstack-protector-guard-offset=xxx
> >
> > riscv: Use -mstack-protector-guard-offset & guard-reg
> > gcc options:
> > -mstack-protector-guard=tls
> > -mstack-protector-guard-reg=tp
> > -mstack-protector-guard-offset=xxx
> >
> > GCC's implementation has been merged:
> > commit c931e8d5a96463427040b0d11f9c4352ac22b2b0
> > Author: Cooper Qu
> > Date: Mon Jul 13 16:15:08 2020 +0800
> >
> > RISC-V: Add support for TLS stack protector canary access
> >
> > In the end, these codes are inserted by gcc before return:
> >
> > * 0xffe00020b396 <+120>: ld a5,1008(tp) # 0x3f0
> > * 0xffe00020b39a <+124>: xor a5,a5,a4
> > * 0xffe00020b39c <+126>: mv a0,s5
> > * 0xffe00020b39e <+128>: bneza5,0xffe00020b61c <_do_fork+766>
> >0xffe00020b3a2 <+132>: ld ra,136(sp)
> >0xffe00020b3a4 <+134>: ld s0,128(sp)
> >0xffe00020b3a6 <+136>: ld s1,120(sp)
> >0xffe00020b3a8 <+138>: ld s2,112(sp)
> >0xffe00020b3aa <+140>: ld s3,104(sp)
> >0xffe00020b3ac <+142>: ld s4,96(sp)
> >0xffe00020b3ae <+144>: ld s5,88(sp)
> >0xffe00020b3b0 <+146>: ld s6,80(sp)
> >0xffe00020b3b2 <+148>: ld s7,72(sp)
> >0xffe00020b3b4 <+150>: addisp,sp,144
> >0xffe00020b3b6 <+152>: ret
> >...
> > * 0xffe00020b61c <+766>: auipc ra,0x7f8
> > * 0xffe00020b620 <+770>: jalr-1764(ra) # 0xffe000a02f38
> > <__stack_chk_fail>
> >
> > Signed-off-by: Guo Ren
>
> Thanks for getting this working! It looks good to me. :)
>
> Reviewed-by: Kees Cook
>
> --
> Kees Cook
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 5/9] riscv: Add kprobes supported
On Fri, Nov 6, 2020 at 9:02 AM Palmer Dabbelt wrote: > > On Sat, 17 Oct 2020 00:06:13 PDT (-0700), guo...@kernel.org wrote: > > From: Guo Ren > > > > This patch enables "kprobe & kretprobe" to work with ftrace > > interface. It utilized software breakpoint as single-step > > mechanism. > > > > Some instructions which can't be single-step executed must be > > simulated in kernel execution slot, such as: branch, jal, auipc, > > la ... > > > > Some instructions should be rejected for probing and we use a > > blacklist to filter, such as: ecall, ebreak, ... > > > > We use ebreak & c.ebreak to replace origin instruction and the > > kprobe handler prepares an executable memory slot for out-of-line > > execution with a copy of the original instruction being probed. > > In execution slot we add ebreak behind original instruction to > > simulate a single-setp mechanism. > > > > The patch is based on packi's work [1] and csky's work [2]. > > - The kprobes_trampoline.S is all from packi's patch > > - The single-step mechanism is new designed for riscv without hw > >single-step trap > > - The simulation codes are from csky > > - Frankly, all codes refer to other archs' implementation > > > > [1] https://lore.kernel.org/linux-riscv/20181113195804.22825-1...@packi.ch/ > > [2] > > https://lore.kernel.org/linux-csky/20200403044150.20562-9-guo...@kernel.org/ > > > > Signed-off-by: Guo Ren > > Co-Developed-by: Patrick Stählin > > Checkpatch says > > WARNING: 'Co-developed-by:' is the preferred signature form > #108: > Co-Developed-by: Patrick Stählin > > WARNING: Co-developed-by: must be immediately followed by Signed-off-by: > #108: > Co-Developed-by: Patrick Stählin > Acked-by: Masami Hiramatsu > > There's some other checkpatch warnings throughout the patch set, but this is > one I'm not supposed to fix up. Can one of you re-post the patches with the > correct DCO? Ok, I'll fix it up in V5 and rebase 5.10-rc2 -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 1/9] RISC-V: Implement ptrace regs and stack API
On Fri, Nov 6, 2020 at 9:03 AM Palmer Dabbelt wrote: > > On Sat, 17 Oct 2020 00:06:09 PDT (-0700), guo...@kernel.org wrote: > > From: Patrick Stählin > > > > Needed for kprobes support. Copied and adapted from arm64 code. > > > > Guo Ren fixup pt_regs type for linux-5.8-rc1. > > > > Signed-off-by: Patrick Stählin > > Signed-off-by: Guo Ren > > Reviewed-by: Pekka Enberg > > Reviewed-by: Zong Li > > Reviewed-by: Masami Hiramatsu > > --- > > arch/riscv/Kconfig | 1 + > > arch/riscv/include/asm/ptrace.h | 29 > > arch/riscv/kernel/ptrace.c | 99 > > + > > 3 files changed, 129 insertions(+) > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index b7821ac..e6424d8b 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -87,6 +87,7 @@ config RISCV > > select SPARSE_IRQ > > select SYSCTL_EXCEPTION_TRACE > > select THREAD_INFO_IN_TASK > > + select HAVE_REGS_AND_STACK_ACCESS_API > > We alphabetize these now -- I'd usually just fix it, but there's a > Signed-off-by issue later in the patch set. ok, I'll fix it and rebase 5.10-rc2. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] stop_machine: Mark functions as notrace
On Tue, Nov 3, 2020 at 11:33 PM Atish Patra wrote:
>
> On Sat, Oct 31, 2020 at 12:42 AM Guo Ren wrote:
> >
> > On Sat, Oct 31, 2020 at 8:28 AM Steven Rostedt wrote:
> > >
> > > On Fri, 30 Oct 2020 14:47:56 -0700
> > > Atish Patra wrote:
> > >
> > > > > Look at arm64, they __kprobes flag and I guess it would also prevent
> > > > > ftrace call site.
> > > > >
> > > >
> > > > Are you sure about that ? __kprobes puts the code in .kprobes.text
> > > > section
> > > > which is under whitelist sections in recordmcount.pl & recordmcount.c.
> > >
> > > Correct, ftrace can trace functions marked with __kprobes. That said,
> > I guess wrong, thx for correct me.
> >
> > > the instruction you are looking at here, is in a file that is
> > > blacklisted from recordmcount.
> > >
> > > CFLAGS_REMOVE_insn.o = $(CC_FLAGS_FTRACE)
> > >
> > > All ftrace flags are removed from the compiling of insn.c, and every
> > > function in that file will not be traced.
> > Yes, arm64 prevents the whole file from ftrace. My patch just use
> > notrace flag setting on some functions.
> >
> > @Atish How do think:
> > CFLAGS_REMOVE_cacheflush.o = $(CC_FLAGS_FTRACE)
> > CFLAGS_REMOVE_sbi.o = $(CC_FLAGS_FTRACE)
> >
>
> Looks good to me. What should be done for copy_to_kernel_nofault ?
> That is also in the calling path.
There is no nops' entry in the prologue of copy_to_kernel_nofault.
>>>>
007c <.LVL6>:
}
7c: 6105addisp,sp,32
7e: 8082ret
0080 :
*
* Safely write to address @dst from the buffer at @src. If a kernel fault
* happens, handle that and return -EFAULT.
*/
long copy_to_user_nofault(void __user *dst, const void *src, size_t size)
{
80: 1101addisp,sp,-32
82: e822sd s0,16(sp)
84: ec06sd ra,24(sp)
86: e426sd s1,8(sp)
88: e04asd s2,0(sp)
8a: 1000addis0,sp,32
<<<<
>>>>
cmd_mm/maccess.o :=
/root/source/riscv-tools/install_64gc/bin/riscv64-unknown-linux-gnu-gcc
-Wp,-MMD,mm/.maccess.o.d -nostdinc -isystem
/root/source/riscv-tools/install_64gc/bin/../lib/gcc/riscv64-unknown-linux-gnu/8.4.0/include
-I./arch/riscv/include -I./arch/riscv/include/generated -I./include
-I./arch/riscv/include/uapi -I./arch/riscv/include/generated/uapi
-I./include/uapi -I./include/generated/uapi -include
./include/linux/kconfig.h -include ./include/linux/compiler_types.h
-D__KERNEL__ ***-DCC_USING_PATCHABLE_FUNCTION_ENTRY*** -Wall -Wundef
-Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
-fno-common -fshort-wchar -fno-PIE
-Werror=implicit-function-declaration -Werror=implicit-int
-Wno-format-security -std=gnu89 -mabi=lp64 -march=rv64imac
-mno-save-restore -DCONFIG_PAGE_OFFSET=0xffe0
-mcmodel=medany -fno-omit-frame-pointer -mstrict-align
-fno-delete-null-pointer-checks -Wno-frame-address
-Wno-format-truncation -Wno-format-overflow -O2
--param=allow-store-data-races=0 -Wframe-larger-than=2048
-fstack-protector-strong -Wno-unused-but-set-variable
-Wimplicit-fallthrough -Wno-unused-const-variable
-fno-omit-frame-pointer -fno-optimize-sibling-calls
-fno-var-tracking-assignments -g ***-fpatchable-function-entry=8***
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign
-Wno-stringop-truncation -Wno-array-bounds -Wno-stringop-overflow
-Wno-restrict -Wno-maybe-uninitialized -fno-strict-overflow
-fno-merge-all-constants -fmerge-constants -fno-stack-check
-fconserve-stack -Werror=date-time -Werror=incompatible-pointer-types
-Werror=designated-init -fmacro-prefix-map=./= -Wno-packed-not-aligned
-DKBUILD_MODFILE='"mm/maccess"' -DKBUILD_BASENAME='"maccess"'
-DKBUILD_MODNAME='"maccess"' -c -o mm/maccess.o mm/maccess.c
<<<<
But copy_from_user_nofault has:
007c <.LVL6>:
}
7c: 6105addisp,sp,32
7e: 8082ret
0080 :
*
* Safely write to address @dst from the buffer at @src. If a kernel fault
* happens, handle that and return -EFAULT.
*/
long copy_to_user_nofault(void __user *dst, const void *src, size_t size)
{
80: 1101addisp,sp,-32
82: e822sd s0,16(sp)
84: ec06sd ra,24(sp)
86: e426sd s1,8(sp)
88: e04asd s2,0(sp)
8a: 1000addis0,sp,32
I think it's a gcc problem, but satisfy our ftrace requirement.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: linux-next: Signed-off-by missing for commit in the csky tree
Sorry, the patch is deprecated. I’ll remove it
Best Regards
Guo Ren
> 在 2020年11月5日，上午4:22，Stephen Rothwell 写道：
>
> Hi all,
>
> Commit
>
> 6257c1904d00 ("drivers/net: Add dwmac-thead added.")
>
> is missing a Signed-off-by from its author.
>
> --
> Cheers,
> Stephen Rothwell
Re: [PATCH v4] riscv: Enable per-task stack canaries
Hi Kees,
Ping? Any feedback?
On Sun, Oct 18, 2020 at 8:39 PM wrote:
>
> From: Guo Ren
>
> This enables the use of per-task stack canary values if GCC has
> support for emitting the stack canary reference relative to the
> value of tp, which holds the task struct pointer in the riscv
> kernel.
>
> After compare arm64 and x86 implementations, seems arm64's is more
> flexible and readable. The key point is how gcc get the offset of
> stack_canary from gs/el0_sp.
>
> x86: Use a fix offset from gs, not flexible.
>
> struct fixed_percpu_data {
> /*
> * GCC hardcodes the stack canary as %gs:40. Since the
> * irq_stack is the object at %gs:0, we reserve the bottom
> * 48 bytes of the irq stack for the canary.
> */
> chargs_base[40]; // :(
> unsigned long stack_canary;
> };
>
> arm64: Use -mstack-protector-guard-offset & guard-reg
> gcc options:
> -mstack-protector-guard=sysreg
> -mstack-protector-guard-reg=sp_el0
> -mstack-protector-guard-offset=xxx
>
> riscv: Use -mstack-protector-guard-offset & guard-reg
> gcc options:
> -mstack-protector-guard=tls
> -mstack-protector-guard-reg=tp
> -mstack-protector-guard-offset=xxx
>
> GCC's implementation has been merged:
> commit c931e8d5a96463427040b0d11f9c4352ac22b2b0
> Author: Cooper Qu
> Date: Mon Jul 13 16:15:08 2020 +0800
>
> RISC-V: Add support for TLS stack protector canary access
>
> In the end, these codes are inserted by gcc before return:
>
> * 0xffe00020b396 <+120>: ld a5,1008(tp) # 0x3f0
> * 0xffe00020b39a <+124>: xor a5,a5,a4
> * 0xffe00020b39c <+126>: mv a0,s5
> * 0xffe00020b39e <+128>: bneza5,0xffe00020b61c <_do_fork+766>
>0xffe00020b3a2 <+132>: ld ra,136(sp)
>0xffe00020b3a4 <+134>: ld s0,128(sp)
>0xffe00020b3a6 <+136>: ld s1,120(sp)
>0xffe00020b3a8 <+138>: ld s2,112(sp)
>0xffe00020b3aa <+140>: ld s3,104(sp)
>0xffe00020b3ac <+142>: ld s4,96(sp)
>0xffe00020b3ae <+144>: ld s5,88(sp)
>0xffe00020b3b0 <+146>: ld s6,80(sp)
>0xffe00020b3b2 <+148>: ld s7,72(sp)
> 0xffe00020b3b4 <+150>: addisp,sp,144
>0xffe00020b3b6 <+152>: ret
>...
> * 0xffe00020b61c <+766>: auipc ra,0x7f8
> * 0xffe00020b620 <+770>: jalr-1764(ra) # 0xffe000a02f38
> <__stack_chk_fail>
>
> Signed-off-by: Guo Ren
> Signed-off-by: Cooper Qu
> Cc: Kees Cook
> ---
> arch/riscv/Kconfig | 7 +++
> arch/riscv/Makefile | 10 ++
> arch/riscv/include/asm/stackprotector.h | 3 ++-
> arch/riscv/kernel/asm-offsets.c | 3 +++
> arch/riscv/kernel/process.c | 2 +-
> 5 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b7821ac..95a0608 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -388,6 +388,13 @@ config CMDLINE_FORCE
>
> endchoice
>
> +config CC_HAVE_STACKPROTECTOR_TLS
> + def_bool $(cc-option,-mstack-protector-guard=tls
> -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
> +
> +config STACKPROTECTOR_PER_TASK
> + def_bool y
> + depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_TLS
> +
> endmenu
>
> config BUILTIN_DTB
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index fb6e37d..f5f8ee9 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -68,6 +68,16 @@ KBUILD_CFLAGS_MODULE += $(call cc-option,-mno-relax)
> # architectures. It's faster to have GCC emit only aligned accesses.
> KBUILD_CFLAGS += $(call cc-option,-mstrict-align)
>
> +ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
> +prepare: stack_protector_prepare
> +stack_protector_prepare: prepare0
> + $(eval KBUILD_CFLAGS += -mstack-protector-guard=tls \
> + -mstack-protector-guard-reg=tp\
> + -mstack-protector-guard-offset=$(shell\
> + awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \
> + include/generated/asm-offsets.h))
> +endif
> +
> # arch specific predefines for sparse
> CHECKFLAGS += -D__riscv -D__riscv_xlen=$(BITS)
>
> diff --git a/arch/riscv/include/asm/stackprotector.h
> b/arch/riscv/incl
Re: [PATCH] stop_machine: Mark functions as notrace
On Sat, Oct 31, 2020 at 8:28 AM Steven Rostedt wrote: > > On Fri, 30 Oct 2020 14:47:56 -0700 > Atish Patra wrote: > > > > Look at arm64, they __kprobes flag and I guess it would also prevent > > > ftrace call site. > > > > > > > Are you sure about that ? __kprobes puts the code in .kprobes.text section > > which is under whitelist sections in recordmcount.pl & recordmcount.c. > > Correct, ftrace can trace functions marked with __kprobes. That said, I guess wrong, thx for correct me. > the instruction you are looking at here, is in a file that is > blacklisted from recordmcount. > > CFLAGS_REMOVE_insn.o = $(CC_FLAGS_FTRACE) > > All ftrace flags are removed from the compiling of insn.c, and every > function in that file will not be traced. Yes, arm64 prevents the whole file from ftrace. My patch just use notrace flag setting on some functions. @Atish How do think: CFLAGS_REMOVE_cacheflush.o = $(CC_FLAGS_FTRACE) CFLAGS_REMOVE_sbi.o = $(CC_FLAGS_FTRACE) -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH] stop_machine: Mark functions as notrace
On Fri, Oct 30, 2020 at 2:46 AM Atish Patra wrote:
>
> On Thu, Oct 29, 2020 at 9:06 AM Guo Ren wrote:
> >
> > On Thu, Oct 29, 2020 at 10:34 AM Zong Li wrote:
> > >
> > > On Thu, Oct 29, 2020 at 8:23 AM Atish Patra wrote:
> > > >
> > > > On Wed, Oct 28, 2020 at 8:44 AM Guo Ren wrote:
> > > > >
> > > > > Hi Zong & Atish,
> > > > >
> > > > > In our 2 harts c910 chip, we found:
> > > > >
> > > > > echo function > /sys/kernel/debug/tracing/current_tracer
> > > > > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > > > > echo function > /sys/kernel/debug/tracing/current_tracer
> > > > > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > > > >
> > > > > Then one core halted at stop_machine_yield:
> > > > > arch_cpu_idle () at arch/riscv/kernel/process.c:39
> > > > > 39 local_irq_enable();
> > > > > (gdb) i th
> > > > > Id Target Id Frame
> > > > > * 1Thread 1 (CPU#0) arch_cpu_idle () at
> > > > > arch/riscv/kernel/process.c:39
> > > > > 2Thread 2 (CPU#1) stop_machine_yield
> > > > > (cpumask=0xffe001371fa8 <__cpu_online_mask>) at
> > > > > ./arch/riscv/include/asm/vdso/processor.h:12
> > > > > (gdb) thread 2
> > > > > [Switching to thread 2 (Thread 2)]
> > > > > #0 stop_machine_yield (cpumask=0xffe001371fa8
> > > > > <__cpu_online_mask>) at ./arch/riscv/include/asm/vdso/processor.h:12
> > > > > 12 __asm__ __volatile__ ("div %0, %0, zero" : "=r"
> > > > > (dummy));
> > > > >
> > > > > With your patch, it's solved. For this patch, I'll give:
> > > > > Tested by: Guo Ren
> > > > >
> > > > > But that's not enough, we still need:
> > > > >
> > > > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > > > index 226ccce..12b8808 100644
> > > > > --- a/arch/riscv/kernel/sbi.c
> > > > > +++ b/arch/riscv/kernel/sbi.c
> > > > > @@ -376,7 +376,7 @@ EXPORT_SYMBOL(sbi_send_ipi);
> > > > > *
> > > > > * Return: None
> > > > > */
> > > > > -void sbi_remote_fence_i(const unsigned long *hart_mask)
> > > > > +void notrace sbi_remote_fence_i(const unsigned long *hart_mask)
> > > > > {
> > > > > __sbi_rfence(SBI_EXT_RFENCE_REMOTE_FENCE_I,
> > > > > hart_mask, 0, 0, 0, 0);
> > > > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > > > index 400b945d..9467d987 100644
> > > > > --- a/arch/riscv/mm/cacheflush.c
> > > > > +++ b/arch/riscv/mm/cacheflush.c
> > > > > @@ -9,12 +9,12 @@
> > > > >
> > > > > #include
> > > > >
> > > > > -static void ipi_remote_fence_i(void *info)
> > > > > +static void notrace ipi_remote_fence_i(void *info)
> > > > > {
> > > > > return local_flush_icache_all();
> > > > > }
> > > > >
> > > > > -void flush_icache_all(void)
> > > > > +void notrace flush_icache_all(void)
> > > > > {
> > > > > if (IS_ENABLED(CONFIG_RISCV_SBI))
> > > > > sbi_remote_fence_i(NULL);
> > > > >
> > > >
> > > > Did you see any issue if these functions are not marked as notrace ?
> > > >
> > > > As per Zong's explanation, the issue was that the other harts already
> > > > fetched the next 2 nops and
> > > > executed 1 while kernel patching replaced other with one of the auipc
> > > > + jalr pair.
> > > >
> > > > @Zong can correct me if I am wrong.
> > > >
> > > > These functions are too far ahead. Can it cause such issues ? If yes,
> > > > then we need to mark each and every function
> > > > that can be invoked from patch_text_nosync and are not inlined.
> > > >
> > > > That includes copy_to_kernel_nofault, __sbi_rfence_v02,
> > > > __sbi_rfence_v02_call, sbi_ecall.
> > > >
> > > > Few of these functions may be inlined by compiler. Can we depend on
>
Re: [PATCH] stop_machine: Mark functions as notrace
On Thu, Oct 29, 2020 at 10:34 AM Zong Li wrote:
>
> On Thu, Oct 29, 2020 at 8:23 AM Atish Patra wrote:
> >
> > On Wed, Oct 28, 2020 at 8:44 AM Guo Ren wrote:
> > >
> > > Hi Zong & Atish,
> > >
> > > In our 2 harts c910 chip, we found:
> > >
> > > echo function > /sys/kernel/debug/tracing/current_tracer
> > > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > > echo function > /sys/kernel/debug/tracing/current_tracer
> > > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > >
> > > Then one core halted at stop_machine_yield:
> > > arch_cpu_idle () at arch/riscv/kernel/process.c:39
> > > 39 local_irq_enable();
> > > (gdb) i th
> > > Id Target Id Frame
> > > * 1Thread 1 (CPU#0) arch_cpu_idle () at
> > > arch/riscv/kernel/process.c:39
> > > 2Thread 2 (CPU#1) stop_machine_yield
> > > (cpumask=0xffe001371fa8 <__cpu_online_mask>) at
> > > ./arch/riscv/include/asm/vdso/processor.h:12
> > > (gdb) thread 2
> > > [Switching to thread 2 (Thread 2)]
> > > #0 stop_machine_yield (cpumask=0xffe001371fa8
> > > <__cpu_online_mask>) at ./arch/riscv/include/asm/vdso/processor.h:12
> > > 12 __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> > >
> > > With your patch, it's solved. For this patch, I'll give:
> > > Tested by: Guo Ren
> > >
> > > But that's not enough, we still need:
> > >
> > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > index 226ccce..12b8808 100644
> > > --- a/arch/riscv/kernel/sbi.c
> > > +++ b/arch/riscv/kernel/sbi.c
> > > @@ -376,7 +376,7 @@ EXPORT_SYMBOL(sbi_send_ipi);
> > > *
> > > * Return: None
> > > */
> > > -void sbi_remote_fence_i(const unsigned long *hart_mask)
> > > +void notrace sbi_remote_fence_i(const unsigned long *hart_mask)
> > > {
> > > __sbi_rfence(SBI_EXT_RFENCE_REMOTE_FENCE_I,
> > > hart_mask, 0, 0, 0, 0);
> > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > index 400b945d..9467d987 100644
> > > --- a/arch/riscv/mm/cacheflush.c
> > > +++ b/arch/riscv/mm/cacheflush.c
> > > @@ -9,12 +9,12 @@
> > >
> > > #include
> > >
> > > -static void ipi_remote_fence_i(void *info)
> > > +static void notrace ipi_remote_fence_i(void *info)
> > > {
> > > return local_flush_icache_all();
> > > }
> > >
> > > -void flush_icache_all(void)
> > > +void notrace flush_icache_all(void)
> > > {
> > > if (IS_ENABLED(CONFIG_RISCV_SBI))
> > > sbi_remote_fence_i(NULL);
> > >
> >
> > Did you see any issue if these functions are not marked as notrace ?
> >
> > As per Zong's explanation, the issue was that the other harts already
> > fetched the next 2 nops and
> > executed 1 while kernel patching replaced other with one of the auipc
> > + jalr pair.
> >
> > @Zong can correct me if I am wrong.
> >
> > These functions are too far ahead. Can it cause such issues ? If yes,
> > then we need to mark each and every function
> > that can be invoked from patch_text_nosync and are not inlined.
> >
> > That includes copy_to_kernel_nofault, __sbi_rfence_v02,
> > __sbi_rfence_v02_call, sbi_ecall.
> >
> > Few of these functions may be inlined by compiler. Can we depend on that ?
> >
> > > Because:
> > > (gdb) bt
> > > #0 flush_icache_all () at arch/riscv/mm/cacheflush.c:20
> > > #1 0xffe00020473a in patch_text_nosync (addr=, insns=
> > > , len=) at arch/riscv/kernel/patch.c:96
> > > #2 0xffe000206792 in ftrace_make_call (rec=,
> > > addr=) at arch/riscv/kernel/ftrace.c:109
> > > #3 0xffe0002c9be4 in __ftrace_replace_code (rec=0xffe01ae40020, e
> > > nable=true) at kernel/trace/ftrace.c:2503
> > > #4 0xffe0002ca092 in ftrace_replace_code (mod_flags= > > out>) at kernel/trace/ftrace.c:2530
> > > #5 0xffe0002ca24a in ftrace_modify_all_code (command=9) at kernel
> > >/trace/ftrace.c:2677
> > > #6 0xffe0002ca2ee in __ftrace_modify_code (data=) at
> > >kernel/trace/ftrace.c:2703
> > > #7 0xffe0002c1390 in multi_cpu_stop (data=0x0) at kernel/stop_machin
> > >
Re: [PATCH] stop_machine: Mark functions as notrace
On Thu, Oct 29, 2020 at 8:23 AM Atish Patra wrote:
>
> On Wed, Oct 28, 2020 at 8:44 AM Guo Ren wrote:
> >
> > Hi Zong & Atish,
> >
> > In our 2 harts c910 chip, we found:
> >
> > echo function > /sys/kernel/debug/tracing/current_tracer
> > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > echo function > /sys/kernel/debug/tracing/current_tracer
> > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> >
> > Then one core halted at stop_machine_yield:
> > arch_cpu_idle () at arch/riscv/kernel/process.c:39
> > 39 local_irq_enable();
> > (gdb) i th
> > Id Target Id Frame
> > * 1Thread 1 (CPU#0) arch_cpu_idle () at arch/riscv/kernel/process.c:39
> > 2Thread 2 (CPU#1) stop_machine_yield
> > (cpumask=0xffe001371fa8 <__cpu_online_mask>) at
> > ./arch/riscv/include/asm/vdso/processor.h:12
> > (gdb) thread 2
> > [Switching to thread 2 (Thread 2)]
> > #0 stop_machine_yield (cpumask=0xffe001371fa8
> > <__cpu_online_mask>) at ./arch/riscv/include/asm/vdso/processor.h:12
> > 12 __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >
> > With your patch, it's solved. For this patch, I'll give:
> > Tested by: Guo Ren
> >
> > But that's not enough, we still need:
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 226ccce..12b8808 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -376,7 +376,7 @@ EXPORT_SYMBOL(sbi_send_ipi);
> > *
> > * Return: None
> > */
> > -void sbi_remote_fence_i(const unsigned long *hart_mask)
> > +void notrace sbi_remote_fence_i(const unsigned long *hart_mask)
> > {
> > __sbi_rfence(SBI_EXT_RFENCE_REMOTE_FENCE_I,
> > hart_mask, 0, 0, 0, 0);
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 400b945d..9467d987 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -9,12 +9,12 @@
> >
> > #include
> >
> > -static void ipi_remote_fence_i(void *info)
> > +static void notrace ipi_remote_fence_i(void *info)
> > {
> > return local_flush_icache_all();
> > }
> >
> > -void flush_icache_all(void)
> > +void notrace flush_icache_all(void)
> > {
> > if (IS_ENABLED(CONFIG_RISCV_SBI))
> > sbi_remote_fence_i(NULL);
> >
>
> Did you see any issue if these functions are not marked as notrace ?
Welcome to Buildroot
buildroot login: root
#
# cat /proc/cpuinfo
processor : 0
hart: 0
isa : rv64imafdcsu
mmu : sv39
#
#
# echo function > /sys/kernel/debug/tracing/current_tracer
[ 45.234334] Unable to handle kernel paging request at virtual
address ffd38ae80900
[ 45.242313] Oops [#1]
[ 45.244600] Modules linked in:
[ 45.247678] CPU: 0 PID: 11 Comm: migration/0 Not tainted
5.9.0-00025-g9b7db83-dirty #215
[ 45.255797] epc: ffe00021689a ra : ffe00021718e sp : ffe01afabb58
[ 45.262955] gp : ffe00136afa0 tp : ffe01af94d00 t0 :
0002
[ 45.270200] t1 : t2 : 0001 s0 :
ffe01afabc08
[ 45.277443] s1 : ffe0013718a8 a0 : a1 :
ffe01afabba8
[ 45.284686] a2 : a3 : a4 :
c4c16ad38ae80900
[ 45.291929] a5 : a6 : a7 :
52464e43
[ 45.299173] s2 : 0001 s3 : ffe000206a60 s4 :
ffe000206a60
[ 45.306415] s5 : 09ec s6 : ffe0013718a8 s7 :
c4c16ad38ae80900
[ 45.313658] s8 : 0004 s9 : 0001 s10:
0001
[ 45.320902] s11: 0003 t3 : 0001 t4 :
d192fe79
[ 45.328144] t5 : b8f8 t6 : 0004
[ 45.333472] status: 00020100 badaddr: ffd38ae80900
cause: 000f
[ 45.341514] ---[ end trace d95102172248fdcf ]---
[ 45.346176] note: migration/0[11] exited with preempt_count 1
(gdb) x /2i $pc
=> 0xffe00021689a <__do_proc_dointvec+196>: sd zero,0(s7)
0xffe00021689e <__do_proc_dointvec+200>: li s11,0
(gdb) bt
#0 __do_proc_dointvec (tbl_data=0x0, table=0xffe01afabba8,
write=0, buffer=0x0, lenp=0x7bf897061f9a0800, ppos=0x4, conv=0x0,
data=0x52464e43) at kernel/sysctl.c:581
#1 0xffe00021718e in do_proc_dointvec (data=,
conv=, ppos=, lenp=,
buffer=, write=, table=)
at kernel/sysctl.c:964
#2 proc_dointvec_minmax (ppos=, lenp=,
buffer=, write=, table=)
at kernel/sysctl.c:964
#3 proc_do_static_key (table=, write=1, b
Re: [PATCH] stop_machine: Mark functions as notrace
Hi Zong & Atish,
In our 2 harts c910 chip, we found:
echo function > /sys/kernel/debug/tracing/current_tracer
echo function_graph > /sys/kernel/debug/tracing/current_tracer
echo function > /sys/kernel/debug/tracing/current_tracer
echo function_graph > /sys/kernel/debug/tracing/current_tracer
Then one core halted at stop_machine_yield:
arch_cpu_idle () at arch/riscv/kernel/process.c:39
39 local_irq_enable();
(gdb) i th
Id Target Id Frame
* 1Thread 1 (CPU#0) arch_cpu_idle () at arch/riscv/kernel/process.c:39
2Thread 2 (CPU#1) stop_machine_yield
(cpumask=0xffe001371fa8 <__cpu_online_mask>) at
./arch/riscv/include/asm/vdso/processor.h:12
(gdb) thread 2
[Switching to thread 2 (Thread 2)]
#0 stop_machine_yield (cpumask=0xffe001371fa8
<__cpu_online_mask>) at ./arch/riscv/include/asm/vdso/processor.h:12
12 __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
With your patch, it's solved. For this patch, I'll give:
Tested by: Guo Ren
But that's not enough, we still need:
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 226ccce..12b8808 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -376,7 +376,7 @@ EXPORT_SYMBOL(sbi_send_ipi);
*
* Return: None
*/
-void sbi_remote_fence_i(const unsigned long *hart_mask)
+void notrace sbi_remote_fence_i(const unsigned long *hart_mask)
{
__sbi_rfence(SBI_EXT_RFENCE_REMOTE_FENCE_I,
hart_mask, 0, 0, 0, 0);
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 400b945d..9467d987 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -9,12 +9,12 @@
#include
-static void ipi_remote_fence_i(void *info)
+static void notrace ipi_remote_fence_i(void *info)
{
return local_flush_icache_all();
}
-void flush_icache_all(void)
+void notrace flush_icache_all(void)
{
if (IS_ENABLED(CONFIG_RISCV_SBI))
sbi_remote_fence_i(NULL);
Because:
(gdb) bt
#0 flush_icache_all () at arch/riscv/mm/cacheflush.c:20
#1 0xffe00020473a in patch_text_nosync (addr=, insns=
, len=) at arch/riscv/kernel/patch.c:96
#2 0xffe000206792 in ftrace_make_call (rec=,
addr=) at arch/riscv/kernel/ftrace.c:109
#3 0xffe0002c9be4 in __ftrace_replace_code (rec=0xffe01ae40020, e
nable=true) at kernel/trace/ftrace.c:2503
#4 0xffe0002ca092 in ftrace_replace_code (mod_flags=) at kernel/trace/ftrace.c:2530
#5 0xffe0002ca24a in ftrace_modify_all_code (command=9) at kernel
/trace/ftrace.c:2677
#6 0xffe0002ca2ee in __ftrace_modify_code (data=) at
kernel/trace/ftrace.c:2703
#7 0xffe0002c1390 in multi_cpu_stop (data=0x0) at kernel/stop_machin
e.c:224
#8 0xffe0002c0fbe in cpu_stopper_thread (cpu=) at kern
el/stop_machine.c:491
#9 0xffe0002343be in smpboot_thread_fn (data=0x0) at kernel/smpboot.
c:165
#10 0xffe00022f894 in kthread (_create=0xffe01af13040) at kern
el/kthread.c:292
#11 0xffe000201fac in handle_exception () at arch/riscv/kernel/entry.S:236
On Wed, Oct 21, 2020 at 3:38 PM Zong Li wrote:
>
> Like the commit cb9d7fd51d9f ("watchdog: Mark watchdog touch functions
> as notrace"), some architectures assume that the stopped CPUs don't make
> function calls to traceable functions when they are in the stopped
> state. For example, it causes unexpected kernel crashed when switching
> tracer on RISC-V.
>
> The following patches added calls to these two functions, fix it by
> adding the notrace annotations.
>
> Fixes: 4ecf0a43e729 ("processor: get rid of cpu_relax_yield")
> Fixes: 366237e7b083 ("stop_machine: Provide RCU quiescent state in
> multi_cpu_stop()")
>
> Signed-off-by: Zong Li
> ---
> kernel/rcu/tree.c | 2 +-
> kernel/stop_machine.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 06895ef85d69..2a52f42f64b6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -409,7 +409,7 @@ bool rcu_eqs_special_set(int cpu)
> *
> * The caller must have disabled interrupts and must not be idle.
> */
> -void rcu_momentary_dyntick_idle(void)
> +notrace void rcu_momentary_dyntick_idle(void)
> {
> int special;
>
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index 865bb0228ab6..890b79cf0e7c 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -178,7 +178,7 @@ static void ack_state(struct multi_stop_data *msdata)
> set_state(msdata, msdata->state + 1);
> }
>
> -void __weak stop_machine_yield(const struct cpumask *cpumask)
> +notrace void __weak stop_machine_yield(const struct cpumask *cpumask)
> {
> cpu_relax();
> }
> --
> 2.28.0
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
Hi Anup, On Sun, Oct 25, 2020 at 5:18 PM Anup Patel wrote: > > On Sat, Oct 24, 2020 at 8:40 AM Guo Ren wrote: > > > > On Fri, Oct 23, 2020 at 8:31 PM Anup Patel wrote: > > > > > > On Fri, Oct 23, 2020 at 3:48 PM wrote: > > > > > > > > From: Guo Ren > > > > > > > > ENABLE and CONTEXT registers contain M & S status for per-hart, so > > > > ref to the specification the correct definition is double to the > > > > current value. > > > > > > > > The value of hart_base and enable_base should be calculated by real > > > > physical hartid not software id. Sometimes the CPU node's > > > > from dts is not equal to the sequence index. > > > > > > > > Signed-off-by: Guo Ren > > > > --- > > > > drivers/irqchip/irq-sifive-plic.c | 12 ++-- > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-sifive-plic.c > > > > b/drivers/irqchip/irq-sifive-plic.c > > > > index eaa3e9f..2e56576 100644 > > > > --- a/drivers/irqchip/irq-sifive-plic.c > > > > +++ b/drivers/irqchip/irq-sifive-plic.c > > > > @@ -44,16 +44,16 @@ > > > > * Each hart context has a vector of interrupt enable bits associated > > > > with it. > > > > * There's one bit for each interrupt source. > > > > */ > > > > -#define ENABLE_BASE0x2000 > > > > -#define ENABLE_PER_HART0x80 > > > > +#define ENABLE_BASE0x2080 > > > > +#define ENABLE_PER_HART0x100 > > > > > > > > /* > > > > * Each hart context has a set of control registers associated with > > > > it. Right > > > > * now there's only two: a source priority threshold over which the > > > > hart will > > > > * take an interrupt, and a register to claim interrupts. > > > > */ > > > > -#define CONTEXT_BASE 0x20 > > > > -#define CONTEXT_PER_HART 0x1000 > > > > +#define CONTEXT_BASE 0x201000 > > > > +#define CONTEXT_PER_HART 0x2000 > > > > #define CONTEXT_THRESHOLD 0x00 > > > > #define CONTEXT_CLAIM 0x04 > > > > > > > > @@ -358,10 +358,10 @@ static int __init plic_init(struct device_node > > > > *node, > > > > cpumask_set_cpu(cpu, >lmask); > > > > handler->present = true; > > > > handler->hart_base = > > > > - priv->regs + CONTEXT_BASE + i * > > > > CONTEXT_PER_HART; > > > > + priv->regs + CONTEXT_BASE + hartid * > > > > CONTEXT_PER_HART; > > > > raw_spin_lock_init(>enable_lock); > > > > handler->enable_base = > > > > - priv->regs + ENABLE_BASE + i * ENABLE_PER_HART; > > > > + priv->regs + ENABLE_BASE + hartid * > > > > ENABLE_PER_HART; > > > > handler->priv = priv; > > > > done: > > > > for (hwirq = 1; hwirq <= nr_irqs; hwirq++) > > > > -- > > > > 2.7.4 > > > > > > > > > > There is no one-to-one mapping between PLIC context and HARTID. Instead, > > > we have many-to-one mapping between PLIC contexts and HARTID. In other > > > words, we have one PLIC context for each interrupt capable mode (i.e. > > > M/S-mode) of each HART. > > > > > > For example, on SiFive Unleashed we have 5 HARTs but HARTID=0 has > > > only M-mode capable of taking interrupts so we have total (1 + 2x4) = 9 > > > PLIC contexts. > > That's OK, but what the bug I want to point out is enable_base & > > context_base should be calculated by 'hartid' not 'i'. > > There is no relation between PLIC context number and HART IDs. The > PLIC context to HART mapping is discovered from the "interrupts-extended" > DT property of PLIC DT node. The "i" in the loop is PLIC context number. > > The PLIC spec does not mandate any ordering/pattern of PLIC context to > HART mappings. Also, the interrupts-extended DT property is generic > enough to represent any kind of PLIC context to HART mappings. > > Your patch breaks SiFive Unl
Re: [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs
On Fri, Oct 23, 2020 at 8:17 PM Anup Patel wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM wrote:
> >
> > From: Guo Ren
> >
> > If "echo 3 > /proc/irq/1/smp_affinity", we want irq 1 could be
> > broadcast to CPU0 & CPU1 and one of them would pick up the irq
> > handler.
> >
> > But current implementation couldn't let multi CPUs process the
> > same IRQ concurrent.
> >
> > Signed-off-by: Guo Ren
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 23 ++-
> > 1 file changed, 6 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c
> > b/drivers/irqchip/irq-sifive-plic.c
> > index 2e56576..0003322 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -114,15 +114,12 @@ static inline void plic_irq_toggle(const struct
> > cpumask *mask,
> > static void plic_irq_unmask(struct irq_data *d)
> > {
> > struct cpumask amask;
> > - unsigned int cpu;
> > struct plic_priv *priv = irq_get_chip_data(d->irq);
> >
> > cpumask_and(, >lmask, cpu_online_mask);
> > - cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > - );
> > - if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > - return;
> > - plic_irq_toggle(cpumask_of(cpu), d, 1);
> > + cpumask_and(, , irq_data_get_affinity_mask(d));
> > +
> > + plic_irq_toggle(, d, 1);
> > }
> >
> > static void plic_irq_mask(struct irq_data *d)
> > @@ -136,24 +133,16 @@ static void plic_irq_mask(struct irq_data *d)
> > static int plic_set_affinity(struct irq_data *d,
> > const struct cpumask *mask_val, bool force)
> > {
> > - unsigned int cpu;
> > struct cpumask amask;
> > struct plic_priv *priv = irq_get_chip_data(d->irq);
> >
> > cpumask_and(, >lmask, mask_val);
> > + cpumask_and(, , cpu_online_mask);
> >
> > - if (force)
> > - cpu = cpumask_first();
> > - else
> > - cpu = cpumask_any_and(, cpu_online_mask);
> > -
> > - if (cpu >= nr_cpu_ids)
> > - return -EINVAL;
> > + irq_data_update_effective_affinity(d, );
> >
> > plic_irq_toggle(>lmask, d, 0);
> > - plic_irq_toggle(cpumask_of(cpu), d, 1);
> > -
> > - irq_data_update_effective_affinity(d, cpumask_of(cpu));
> > + plic_irq_toggle(, d, 1);
> >
> > return IRQ_SET_MASK_OK_DONE;
> > }
> > --
> > 2.7.4
> >
>
> In the past, a similar patch was proposed by Zong Li (SiFive). We
> have good reasons to not allow multiple CPUs handle the same IRQ.
>
> Refer, https://lkml.org/lkml/2020/4/26/201
>
> NACK from my side.
Thx for sharing the information, I agree with Zong Li & Greentime's
aspect: Don't limit the usage of PLIC! We could let (one hart -> one
irq) as default.
I also agree that using irq broadcast indiscriminately can cause
performance problems. (Anup and Mark Z's view)
I think you've discussed enough at that time and l won't persist the patch.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
On Fri, Oct 23, 2020 at 8:31 PM Anup Patel wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM wrote:
> >
> > From: Guo Ren
> >
> > ENABLE and CONTEXT registers contain M & S status for per-hart, so
> > ref to the specification the correct definition is double to the
> > current value.
> >
> > The value of hart_base and enable_base should be calculated by real
> > physical hartid not software id. Sometimes the CPU node's
> > from dts is not equal to the sequence index.
> >
> > Signed-off-by: Guo Ren
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c
> > b/drivers/irqchip/irq-sifive-plic.c
> > index eaa3e9f..2e56576 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -44,16 +44,16 @@
> > * Each hart context has a vector of interrupt enable bits associated with
> > it.
> > * There's one bit for each interrupt source.
> > */
> > -#define ENABLE_BASE0x2000
> > -#define ENABLE_PER_HART0x80
> > +#define ENABLE_BASE0x2080
> > +#define ENABLE_PER_HART0x100
> >
> > /*
> > * Each hart context has a set of control registers associated with it.
> > Right
> > * now there's only two: a source priority threshold over which the hart
> > will
> > * take an interrupt, and a register to claim interrupts.
> > */
> > -#define CONTEXT_BASE 0x20
> > -#define CONTEXT_PER_HART 0x1000
> > +#define CONTEXT_BASE 0x201000
> > +#define CONTEXT_PER_HART 0x2000
> > #define CONTEXT_THRESHOLD 0x00
> > #define CONTEXT_CLAIM 0x04
> >
> > @@ -358,10 +358,10 @@ static int __init plic_init(struct device_node *node,
> > cpumask_set_cpu(cpu, >lmask);
> > handler->present = true;
> > handler->hart_base =
> > - priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> > + priv->regs + CONTEXT_BASE + hartid *
> > CONTEXT_PER_HART;
> > raw_spin_lock_init(>enable_lock);
> > handler->enable_base =
> > - priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> > + priv->regs + ENABLE_BASE + hartid * ENABLE_PER_HART;
> > handler->priv = priv;
> > done:
> > for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> > --
> > 2.7.4
> >
>
> There is no one-to-one mapping between PLIC context and HARTID. Instead,
> we have many-to-one mapping between PLIC contexts and HARTID. In other
> words, we have one PLIC context for each interrupt capable mode (i.e.
> M/S-mode) of each HART.
>
> For example, on SiFive Unleashed we have 5 HARTs but HARTID=0 has
> only M-mode capable of taking interrupts so we have total (1 + 2x4) = 9
> PLIC contexts.
That's OK, but what the bug I want to point out is enable_base &
context_base should be calculated by 'hartid' not 'i'.
For example, how we deal with below dts configuration:
cpus {
#address-cells = <1>;
#size-cells = <0>;
timebase-frequency = <300>;
cpu@0 {
device_type = "cpu";
reg = <2>; //* different from index
status = "okay";
compatible = "riscv";
riscv,isa = "rv64imafdcsu";
mmu-type = "riscv,sv39";
cpu0_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu@1 {
device_type = "cpu";
reg = <3>; //* different from index
status = "fail";
compatible = "riscv";
riscv,isa = "rv64imafdcsu";
mmu-type = "riscv,sv39";
cpu1_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
Re: [PATCH v4 9/9] riscv: Fixup lockdep_assert_held(_mutex) in patch_insn_write
Why? funca: nop addi sp,sp, -8 nop sd ra, 0(sp) nop, nop auipc ra, 0x? nop, nop -> jalr -?(ra) nop ld ra,0(sp) nop addi sp,sp, 8 When CPU0 has only changed half of them, CPU1 call funca then CPU1 broken. On Wed, Oct 21, 2020 at 4:41 AM Steven Rostedt wrote: > > On Tue, 20 Oct 2020 19:18:01 +0800 > Guo Ren wrote: > > > > What platform are you testing ? We are seeing a crash while enabling > > > any of the tracers multiple times > > > on Qemu/HiFive Unleashed. > > I use qemu for testing. I've changed dynamic ftrace mechanism from > > mcount to -fpatchable-entry. > > > > The problem is made by the lockdep checking of text_mutex. > > If you are switching to "patchable-entry" you shouldn't need to use > stop_machine for the updates. No? > > -- Steve -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 9/9] riscv: Fixup lockdep_assert_held(_mutex) in patch_insn_write
On Tue, Oct 20, 2020 at 2:54 AM Atish Patra wrote: > > On Sat, Oct 17, 2020 at 12:07 AM wrote: > > > > From: Guo Ren > > > > It will cause warning messages: > > echo function_graph > /sys/kernel/debug/tracing/current_tracer > > [ 47.691397] [ cut here ] > > [ 47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 > > patch_insn_write+0x182/0x19a > > [ 47.694483] Modules linked in: > > [ 47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted > > 5.9.0-11367-g1054335 #132 > > [ 47.698228] epc: ffe000204530 ra : ffe00020452c sp : > > ffe0023ffc20 gp : ffe0013e1fe0 tp : ffe0023e4e00 t0 : > > > > [ 47.701872] t1 : 000e t2 : 001b s0 : > > ffe0023ffc70 s1 : ffe000206850 a0 : a1 : > > > > [ 47.705550] a2 : a3 : ffe03af7c5e8 a4 : > > a5 : a6 : a7 : > > 150b02d8 > > [ 47.709159] s2 : 0008 s3 : 0858 s4 : > > ffe0023ffc98 s5 : 0850 s6 : 0003 s7 : > > 0002 > > [ 47.714135] s8 : 0004 s9 : 0001 s10: > > 0001 s11: 0003 t3 : e000 t4 : > > 00d86254 > > [ 47.716574] t5 : 0005 t6 : 0004status: > > 0100 badaddr: cause: 0003 > > [ 47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted > > 5.9.0-11367-g1054335 #132 > > [ 47.722074] Call Trace: > > [ 47.722561] [] walk_stackframe+0x0/0xc2 > > [ 47.724608] [] show_stack+0x46/0x52 > > [ 47.726246] [] dump_stack+0x90/0xb6 > > [ 47.727672] [] __warn+0x98/0xfa > > [ 47.729131] [] report_bug+0xaa/0x11e > > [ 47.730624] [] do_trap_break+0x96/0xfe > > [ 47.732448] [] ret_from_exception+0x0/0x14 > > [ 47.734341] [] patch_insn_write+0x17e/0x19a > > [ 47.737235] irq event stamp: 39 > > [ 47.738521] hardirqs last enabled at (39): [] > > _save_context+0xa2/0xe6 > > [ 47.741055] hardirqs last disabled at (38): [] > > multi_cpu_stop+0x130/0x166 > > [ 47.743551] softirqs last enabled at (0): [] > > copy_process+0x430/0x1316 > > [ 47.746031] softirqs last disabled at (0): [<>] 0x0 > > [ 47.748617] ---[ end trace 88a1054faa6524ef ]--- > > > > Because the path of stop_machine(__ftrace_modify_code)-> > > ftrace_modify_all_code->...->patch_insn_write has no pair of > > lock text_mutex, so we shouldn't put assert here. > > > > What platform are you testing ? We are seeing a crash while enabling > any of the tracers multiple times > on Qemu/HiFive Unleashed. I use qemu for testing. I've changed dynamic ftrace mechanism from mcount to -fpatchable-entry. The problem is made by the lockdep checking of text_mutex. > > http://lists.infradead.org/pipermail/linux-riscv/2020-October/002354.html > > As per the thread, Zong agreed to take a look into that. > @Zong Li : Do you have any update regarding the ftrace issue. > > > Signed-off-by: Guo Ren > > --- > > arch/riscv/kernel/patch.c | 7 --- > > 1 file changed, 7 deletions(-) > > > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > > index 0b55287..2ee5063 100644 > > --- a/arch/riscv/kernel/patch.c > > +++ b/arch/riscv/kernel/patch.c > > @@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void > > *insn, size_t len) > > bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > > > PAGE_SIZE; > > int ret; > > > > - /* > > -* Before reaching here, it was expected to lock the text_mutex > > -* already, so we don't need to give another lock here and could > > -* ensure that it was safe between each cores. > > -*/ > > - lockdep_assert_held(_mutex); > > - > > if (across_pages) > > patch_map(addr + len, FIX_TEXT_POKE1); > > > > -- > > 2.7.4 > > > > > > ___ > > linux-riscv mailing list > > linux-ri...@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > -- > Regards, > Atish -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 9/9] riscv: Fixup lockdep_assert_held(_mutex) in patch_insn_write
On Mon, Oct 19, 2020 at 4:36 PM Masami Hiramatsu wrote:
>
> On Sat, 17 Oct 2020 07:06:17 +
> guo...@kernel.org wrote:
>
> > From: Guo Ren
> >
> > It will cause warning messages:
> > echo function_graph > /sys/kernel/debug/tracing/current_tracer
> > [ 47.691397] [ cut here ]
> > [ 47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63
> > patch_insn_write+0x182/0x19a
> > [ 47.694483] Modules linked in:
> > [ 47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted
> > 5.9.0-11367-g1054335 #132
> > [ 47.698228] epc: ffe000204530 ra : ffe00020452c sp :
> > ffe0023ffc20 gp : ffe0013e1fe0 tp : ffe0023e4e00 t0 :
> >
> > [ 47.701872] t1 : 000e t2 : 001b s0 :
> > ffe0023ffc70 s1 : ffe000206850 a0 : a1 :
> >
> > [ 47.705550] a2 : a3 : ffe03af7c5e8 a4 :
> > a5 : a6 : a7 :
> > 150b02d8
> > [ 47.709159] s2 : 0008 s3 : 0858 s4 :
> > ffe0023ffc98 s5 : 0850 s6 : 0003 s7 :
> > 0002
> > [ 47.714135] s8 : 0004 s9 : 0001 s10:
> > 0001 s11: 0003 t3 : e000 t4 :
> > 00d86254
> > [ 47.716574] t5 : 0005 t6 : 0004status:
> > 0100 badaddr: cause: 0003
> > [ 47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted
> > 5.9.0-11367-g1054335 #132
> > [ 47.722074] Call Trace:
> > [ 47.722561] [] walk_stackframe+0x0/0xc2
> > [ 47.724608] [] show_stack+0x46/0x52
> > [ 47.726246] [] dump_stack+0x90/0xb6
> > [ 47.727672] [] __warn+0x98/0xfa
> > [ 47.729131] [] report_bug+0xaa/0x11e
> > [ 47.730624] [] do_trap_break+0x96/0xfe
> > [ 47.732448] [] ret_from_exception+0x0/0x14
> > [ 47.734341] [] patch_insn_write+0x17e/0x19a
> > [ 47.737235] irq event stamp: 39
> > [ 47.738521] hardirqs last enabled at (39): []
> > _save_context+0xa2/0xe6
> > [ 47.741055] hardirqs last disabled at (38): []
> > multi_cpu_stop+0x130/0x166
> > [ 47.743551] softirqs last enabled at (0): []
> > copy_process+0x430/0x1316
> > [ 47.746031] softirqs last disabled at (0): [<>] 0x0
> > [ 47.748617] ---[ end trace 88a1054faa6524ef ]---
> >
> > Because the path of stop_machine(__ftrace_modify_code)->
> > ftrace_modify_all_code->...->patch_insn_write has no pair of
> > lock text_mutex, so we shouldn't put assert here.
>
> Hm, are there any chance to get the text_mutex in that path?
> There are ftrace_arch_code_modify_prepare/post_process() in
> arch/riscv/kernel/ftrace.c,
> aren't those used?
I found ftrace_run_update_code:
- ftrace_arch_code_modify_prepare()-> mutex_lock(_mutex)
- stop_machine(__ftrace_modify_code, , NULL); - >
__ftrace_modify_code -> patch_insn_write -> lockdep_assert_held ->
lockdep_is_held
- ftrace_arch_code_modify_post_process()-> mutex_unlock(_mutex)
Because of stop_machine, __ftrace_modify_code and
ftrace_run_update_code are not the same task. This will cause
lockdep_is_held return 0.
ref to __lock_is_held:
volatile int __lock_is_held(const struct lockdep_map *lock, int read)
{
struct task_struct *curr = current; // current is from
stop_machine, not ftrace_run_update_code. So never match!
int i;
for (i = 0; i < curr->lockdep_depth; i++) {
struct held_lock *hlock = curr->held_locks + i;
if (match_held_lock(hlock, lock)) {
if (read == -1 || hlock->read == read)
return 1;
return 0;
}
}
return 0;
}
Does it seem a comm issue? I'm not so expert on lockdep mechanism.
>
> Thank you,
>
> >
> > Signed-off-by: Guo Ren
> > ---
> > arch/riscv/kernel/patch.c | 7 ---
> > 1 file changed, 7 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index 0b55287..2ee5063 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void
> > *insn, size_t len)
> > bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) >
> > PAGE_SIZE;
> > int ret;
> >
> > - /*
> > - * Before reaching here, it was expected to lock the text_mutex
> > - * already, so we don't need to give another lock here and could
> > - * ensure that it was safe between each cores.
> > - */
> > - lockdep_assert_held(_mutex);
> > -
> > if (across_pages)
> > patch_map(addr + len, FIX_TEXT_POKE1);
> >
> > --
> > 2.7.4
> >
>
>
> --
> Masami Hiramatsu
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH v4 9/9] riscv: Fixup lockdep_assert_held(_mutex) in patch_insn_write
Good Idea! I'll try :P On Mon, Oct 19, 2020 at 4:36 PM Masami Hiramatsu wrote: > > On Sat, 17 Oct 2020 07:06:17 + > guo...@kernel.org wrote: > > > From: Guo Ren > > > > It will cause warning messages: > > echo function_graph > /sys/kernel/debug/tracing/current_tracer > > [ 47.691397] [ cut here ] > > [ 47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 > > patch_insn_write+0x182/0x19a > > [ 47.694483] Modules linked in: > > [ 47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted > > 5.9.0-11367-g1054335 #132 > > [ 47.698228] epc: ffe000204530 ra : ffe00020452c sp : > > ffe0023ffc20 gp : ffe0013e1fe0 tp : ffe0023e4e00 t0 : > > > > [ 47.701872] t1 : 000e t2 : 001b s0 : > > ffe0023ffc70 s1 : ffe000206850 a0 : a1 : > > > > [ 47.705550] a2 : a3 : ffe03af7c5e8 a4 : > > a5 : a6 : a7 : > > 150b02d8 > > [ 47.709159] s2 : 0008 s3 : 0858 s4 : > > ffe0023ffc98 s5 : 0850 s6 : 0003 s7 : > > 0002 > > [ 47.714135] s8 : 0004 s9 : 0001 s10: > > 0001 s11: 0003 t3 : e000 t4 : > > 00d86254 > > [ 47.716574] t5 : 0005 t6 : 0004status: > > 0100 badaddr: cause: 0003 > > [ 47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted > > 5.9.0-11367-g1054335 #132 > > [ 47.722074] Call Trace: > > [ 47.722561] [] walk_stackframe+0x0/0xc2 > > [ 47.724608] [] show_stack+0x46/0x52 > > [ 47.726246] [] dump_stack+0x90/0xb6 > > [ 47.727672] [] __warn+0x98/0xfa > > [ 47.729131] [] report_bug+0xaa/0x11e > > [ 47.730624] [] do_trap_break+0x96/0xfe > > [ 47.732448] [] ret_from_exception+0x0/0x14 > > [ 47.734341] [] patch_insn_write+0x17e/0x19a > > [ 47.737235] irq event stamp: 39 > > [ 47.738521] hardirqs last enabled at (39): [] > > _save_context+0xa2/0xe6 > > [ 47.741055] hardirqs last disabled at (38): [] > > multi_cpu_stop+0x130/0x166 > > [ 47.743551] softirqs last enabled at (0): [] > > copy_process+0x430/0x1316 > > [ 47.746031] softirqs last disabled at (0): [<>] 0x0 > > [ 47.748617] ---[ end trace 88a1054faa6524ef ]--- > > > > Because the path of stop_machine(__ftrace_modify_code)-> > > ftrace_modify_all_code->...->patch_insn_write has no pair of > > lock text_mutex, so we shouldn't put assert here. > > Hm, are there any chance to get the text_mutex in that path? > There are ftrace_arch_code_modify_prepare/post_process() in > arch/riscv/kernel/ftrace.c, > aren't those used? > > Thank you, > > > > > Signed-off-by: Guo Ren > > --- > > arch/riscv/kernel/patch.c | 7 --- > > 1 file changed, 7 deletions(-) > > > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > > index 0b55287..2ee5063 100644 > > --- a/arch/riscv/kernel/patch.c > > +++ b/arch/riscv/kernel/patch.c > > @@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void > > *insn, size_t len) > > bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > > > PAGE_SIZE; > > int ret; > > > > - /* > > - * Before reaching here, it was expected to lock the text_mutex > > - * already, so we don't need to give another lock here and could > > - * ensure that it was safe between each cores. > > - */ > > - lockdep_assert_held(_mutex); > > - > > if (across_pages) > > patch_map(addr + len, FIX_TEXT_POKE1); > > > > -- > > 2.7.4 > > > > > -- > Masami Hiramatsu -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 2/2] riscv: Fixup static_obj() fail v2
On Thu, Oct 8, 2020 at 11:54 AM Palmer Dabbelt wrote:
>
> On Wed, 07 Oct 2020 08:08:33 PDT (-0700), guo...@kernel.org wrote:
> > From: Guo Ren
> >
> > v1 is commit: 6184358da0004c8fd940afda6c0a0fa4027dc911 which has
> > been reverted.
> >
> > When enable LOCKDEP, static_obj() will cause error:
> >
> > [0.067192] INFO: trying to register non-static key.
> > [0.067325] the code is fine but needs lockdep annotation.
> > [0.067449] turning off the locking correctness validator.
> > [0.067718] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc7-dirty #44
> > [0.067945] Call Trace:
> > [0.068369] [] walk_stackframe+0x0/0xa4
> > [0.068506] [] show_stack+0x2a/0x34
> > [0.068631] [] dump_stack+0x94/0xca
> > [0.068757] [] register_lock_class+0x5b8/0x5bc
> > [0.068969] [] __lock_acquire+0x6c/0x1d5c
> > [0.069101] [] lock_acquire+0xae/0x312
> > [0.069228] [] _raw_spin_lock_irqsave+0x40/0x5a
> > [0.069357] [] complete+0x1e/0x50
> > [0.069479] [] rest_init+0x1b0/0x28a
> > [0.069660] [] 0xffe016a2
> > [0.069779] [] 0xffe01b84
> > [0.069953] [] 0xffe01092
> >
> > Because some __initdata static variables is before _stext:
> >
> > static int static_obj(const void *obj)
> > {
> > unsigned long start = (unsigned long) &_stext,
> > end = (unsigned long) &_end,
> > addr = (unsigned long) obj;
> >
> > /*
> > * static variable?
> > */
> > if ((addr >= start) && (addr < end))
> > return 1;
> >
> > if (arch_is_kernel_data(addr))
> > return 1;
> >
> > We could implement arch_is_kernel_data to fixup it.
> >
> > Link:
> > https://lore.kernel.org/linux-riscv/1593266228-61125-1-git-send-email-guo...@kernel.org/T/#t
> > Signed-off-by: Guo Ren
> > Reported-by: Aurelien Jarno
> > Cc: Palmer Dabbelt
> > Cc: Atish Patra
> > Cc: Andreas Schwab
> > Cc: Aurelien Jarno
> > ---
> > arch/riscv/include/asm/sections.h | 20
> > arch/riscv/kernel/setup.c | 9 +
> > 2 files changed, 29 insertions(+)
> > create mode 100644 arch/riscv/include/asm/sections.h
> >
> > diff --git a/arch/riscv/include/asm/sections.h
> > b/arch/riscv/include/asm/sections.h
> > new file mode 100644
> > index ..2317b9e
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/sections.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _ASM_RISCV_SECTIONS_H
> > +#define _ASM_RISCV_SECTIONS_H
> > +
> > +#define arch_is_kernel_data arch_is_kernel_data
> > +
> > +#include
> > +
> > +extern bool init_mem_is_free;
> > +
> > +static inline int arch_is_kernel_data(unsigned long addr)
> > +{
> > + if (init_mem_is_free)
> > + return 0;
> > +
> > + return addr >= (unsigned long)__init_begin &&
> > + addr < (unsigned long)__init_end;
> > +}
> > +#endif /* _ASM_RISCV_SECTIONS_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 2c6dd32..9ebd5eb4 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -112,3 +113,11 @@ static int __init topology_init(void)
> > return 0;
> > }
> > subsys_initcall(topology_init);
> > +
> > +bool init_mem_is_free = false;
> > +
> > +void free_initmem(void)
> > +{
> > + free_initmem_default(POISON_FREE_INITMEM);
> > + init_mem_is_free = true;
> > +}
>
> I'm a bit confused as to what you're trying to do here. Yesterday I got
> another version of this patch set that moves init data around, today a
> different one. Yesterday's is tested and simpler, and given it's so late in
> the process I'm inclined to take that as I don't want to break anything.
>
> Right now I have
>
> 84814460eef9 ("riscv: Fixup bootup failure with HARDENED_USERCOPY")
> a78c6f5956a9 ("RISC-V: Make sure memblock reserves the memory containing DT")
> 549738f15da0 ("Linux 5.9-rc8")
>
> Unless there's some functional bug, that's what I'm going to send out for 5.9
> -- I'm not all that worried about lacking the ability to free init data. The
> above seems like fine 5.10 material.
>
> Let me know if I'm missing something here.
84814460eef9 could resolve the problem and Atish ask for any other
idea? So It's another choice, I forgot RFC in prefix.
6184358da0004c8fd940afda6c0a0fa4027dc911("riscv: Fixup static_obj()
fail") is a sloppy patch that introduces another problem. Sorry about
that.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Re: [PATCH 2/2] riscv: Fixup static_obj() fail v2
On Thu, Oct 8, 2020 at 3:46 AM Atish Patra wrote:
>
> On Wed, Oct 7, 2020 at 8:09 AM wrote:
> >
> > From: Guo Ren
> >
> > v1 is commit: 6184358da0004c8fd940afda6c0a0fa4027dc911 which has
> > been reverted.
> >
> > When enable LOCKDEP, static_obj() will cause error:
> >
> > [0.067192] INFO: trying to register non-static key.
> > [0.067325] the code is fine but needs lockdep annotation.
> > [0.067449] turning off the locking correctness validator.
> > [0.067718] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc7-dirty #44
> > [0.067945] Call Trace:
> > [0.068369] [] walk_stackframe+0x0/0xa4
> > [0.068506] [] show_stack+0x2a/0x34
> > [0.068631] [] dump_stack+0x94/0xca
> > [0.068757] [] register_lock_class+0x5b8/0x5bc
> > [0.068969] [] __lock_acquire+0x6c/0x1d5c
> > [0.069101] [] lock_acquire+0xae/0x312
> > [0.069228] [] _raw_spin_lock_irqsave+0x40/0x5a
> > [0.069357] [] complete+0x1e/0x50
> > [0.069479] [] rest_init+0x1b0/0x28a
> > [0.069660] [] 0xffe016a2
> > [0.069779] [] 0xffe01b84
> > [0.069953] [] 0xffe01092
> >
> > Because some __initdata static variables is before _stext:
> >
> > static int static_obj(const void *obj)
> > {
> > unsigned long start = (unsigned long) &_stext,
> > end = (unsigned long) &_end,
> > addr = (unsigned long) obj;
> >
> > /*
> > * static variable?
> > */
> > if ((addr >= start) && (addr < end))
> > return 1;
> >
> > if (arch_is_kernel_data(addr))
> > return 1;
> >
> > We could implement arch_is_kernel_data to fixup it.
> >
> > Link:
> > https://lore.kernel.org/linux-riscv/1593266228-61125-1-git-send-email-guo...@kernel.org/T/#t
> > Signed-off-by: Guo Ren
> > Reported-by: Aurelien Jarno
> > Cc: Palmer Dabbelt
> > Cc: Atish Patra
> > Cc: Andreas Schwab
> > Cc: Aurelien Jarno
> > ---
> > arch/riscv/include/asm/sections.h | 20
> > arch/riscv/kernel/setup.c | 9 +
> > 2 files changed, 29 insertions(+)
> > create mode 100644 arch/riscv/include/asm/sections.h
> >
> > diff --git a/arch/riscv/include/asm/sections.h
> > b/arch/riscv/include/asm/sections.h
> > new file mode 100644
> > index ..2317b9e
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/sections.h
>
> You may want to rebase it on top of for-next as UEFI series also adds this
> file.
ok
> .
>
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef _ASM_RISCV_SECTIONS_H
> > +#define _ASM_RISCV_SECTIONS_H
> > +
> > +#define arch_is_kernel_data arch_is_kernel_data
> > +
> > +#include
> > +
> > +extern bool init_mem_is_free;
> > +
> > +static inline int arch_is_kernel_data(unsigned long addr)
> > +{
> > + if (init_mem_is_free)
> > + return 0;
> > +
> > + return addr >= (unsigned long)__init_begin &&
> > + addr < (unsigned long)__init_end;
> > +}
> > +#endif /* _ASM_RISCV_SECTIONS_H */
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 2c6dd32..9ebd5eb4 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -112,3 +113,11 @@ static int __init topology_init(void)
> > return 0;
> > }
> > subsys_initcall(topology_init);
> > +
> > +bool init_mem_is_free = false;
> > +
> > +void free_initmem(void)
> > +{
> > + free_initmem_default(POISON_FREE_INITMEM);
> > + init_mem_is_free = true;
> > +}
> > --
> > 2.7.4
> >
>
> Looks good. Much cleaner than the first approach.
> FYI: I am still looking into separating __init text & data so that
> different permissions can be applied to them.
> With this patch, we can just separate it on top of _stext.
>
>
> Reviewed-by: Atish Patra
>
> --
> Regards,
> Atish
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
