LKML Archive on lore.kernel.org
 help / Atom feed
* RISC-V: Don't use a global include guard for uapi/asm/syscalls.
@ 2018-08-09 22:19 Palmer Dabbelt
  2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
  2018-08-09 22:19 ` [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h Palmer Dabbelt
  0 siblings, 2 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-08-09 22:19 UTC (permalink / raw)
  To: Christoph Hellwig, linux
  Cc: Palmer Dabbelt, aou, tklauser, Arnd Bergmann, Andrew Waterman,
	linux, dan.carpenter, linux-riscv, linux-kernel

It turns out that we weren't actually hooking sys_riscv_flush_icache
into the syscall table, which results in any flush_icache() call that
escapes the vDSO to silently do nothing.

Changes since v2:

* sys_riscv_flush_icache actually flushes the icache when SMP=n.  Thanks
  to Andrew for pointing out the I screwed it up!

Changes since v1:

* sys_riscv_flush_icache is now defined even when SMP=n, which allows
  this patch set to build against SMP=n and SMP=y.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-09 22:19 RISC-V: Don't use a global include guard for uapi/asm/syscalls Palmer Dabbelt
@ 2018-08-09 22:19 ` Palmer Dabbelt
  2018-08-10  8:38   ` Christoph Hellwig
                     ` (2 more replies)
  2018-08-09 22:19 ` [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h Palmer Dabbelt
  1 sibling, 3 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-08-09 22:19 UTC (permalink / raw)
  To: Christoph Hellwig, linux
  Cc: Palmer Dabbelt, aou, tklauser, Arnd Bergmann, Andrew Waterman,
	linux, dan.carpenter, linux-riscv, linux-kernel,
	Christoph Hellwig

This would be necessary to make non-SMP builds work, but there is
another error in the implementation of our syscall linkage that actually
just causes sys_riscv_flush_icache to never build.  I've build tested
this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
normal.

CC: Christoph Hellwig <hch@infradead.org>
CC: Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20180809055830.GA17533@infradead.org>
In-Reply-To: <20180809132612.GA31058@roeck-us.net>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/include/asm/vdso.h |  2 --
 arch/riscv/kernel/sys_riscv.c | 12 ++++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/vdso.h b/arch/riscv/include/asm/vdso.h
index 541544d64c33..ec6180a4b55d 100644
--- a/arch/riscv/include/asm/vdso.h
+++ b/arch/riscv/include/asm/vdso.h
@@ -38,8 +38,6 @@ struct vdso_data {
 	(void __user *)((unsigned long)(base) + __vdso_##name);			\
 })
 
-#ifdef CONFIG_SMP
 asmlinkage long sys_riscv_flush_icache(uintptr_t, uintptr_t, uintptr_t);
-#endif
 
 #endif /* _ASM_RISCV_VDSO_H */
diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
index f7181ed8aafc..568026ccf6e8 100644
--- a/arch/riscv/kernel/sys_riscv.c
+++ b/arch/riscv/kernel/sys_riscv.c
@@ -48,7 +48,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
 }
 #endif /* !CONFIG_64BIT */
 
-#ifdef CONFIG_SMP
 /*
  * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
  * having a direct 'fence.i' instruction available to userspace (which we
@@ -66,15 +65,24 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
 SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
 	uintptr_t, flags)
 {
+#ifdef CONFIG_SMP
 	struct mm_struct *mm = current->mm;
 	bool local = (flags & SYS_RISCV_FLUSH_ICACHE_LOCAL) != 0;
+#endif
 
 	/* Check the reserved flags. */
 	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
 		return -EINVAL;
 
+	/*
+	 * Without CONFIG_SMP flush_icache_mm is a just a flush_icache_all(),
+	 * which generates unused variable warnings all over this function.
+	 */
+#ifdef CONFIG_SMP
 	flush_icache_mm(mm, local);
+#else
+	flush_icache_all();
+#endif
 
 	return 0;
 }
-#endif
-- 
2.16.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h
  2018-08-09 22:19 RISC-V: Don't use a global include guard for uapi/asm/syscalls Palmer Dabbelt
  2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
@ 2018-08-09 22:19 ` Palmer Dabbelt
  2018-08-10 14:07   ` Guenter Roeck
  2018-08-14 13:40   ` Christoph Hellwig
  1 sibling, 2 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-08-09 22:19 UTC (permalink / raw)
  To: Christoph Hellwig, linux
  Cc: Palmer Dabbelt, aou, tklauser, Arnd Bergmann, Andrew Waterman,
	linux, dan.carpenter, linux-riscv, linux-kernel, Marcus Comstedt

This file is expected to be included multiple times in the same file in
order to allow the __SYSCALL macro to generate system call tables.  With
a global include guard we end up missing __NR_riscv_flush_icache in the
syscall table, which results in icache flushes that escape the vDSO call
to not actually do anything.

The fix is to move to per-#define include guards, which allows the
system call tables to actually be populated.  Thanks to Macrus Comstedt
for finding and fixing the bug!

I also went ahead and fixed the SPDX header to use a //-style comment,
which I've been told is the canonical way to do it.

Cc: Marcus Comstedt <marcus@mc.pp.se>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/include/asm/unistd.h        |  5 +++++
 arch/riscv/include/uapi/asm/syscalls.h | 15 +++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
index 080fb28061de..0caea01d5cca 100644
--- a/arch/riscv/include/asm/unistd.h
+++ b/arch/riscv/include/asm/unistd.h
@@ -11,6 +11,11 @@
  *   GNU General Public License for more details.
  */
 
+/*
+ * There is explicitly no include guard here because this file is expected to
+ * be included multiple times.  See uapi/asm/syscalls.h for more info.
+ */
+
 #define __ARCH_WANT_SYS_CLONE
 #include <uapi/asm/unistd.h>
 #include <uapi/asm/syscalls.h>
diff --git a/arch/riscv/include/uapi/asm/syscalls.h b/arch/riscv/include/uapi/asm/syscalls.h
index 818655b0d535..690beb002d1d 100644
--- a/arch/riscv/include/uapi/asm/syscalls.h
+++ b/arch/riscv/include/uapi/asm/syscalls.h
@@ -1,10 +1,13 @@
-/* SPDX-License-Identifier: GPL-2.0 */
+// SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (C) 2017 SiFive
+ * Copyright (C) 2017-2018 SiFive
  */
 
-#ifndef _ASM__UAPI__SYSCALLS_H
-#define _ASM__UAPI__SYSCALLS_H
+/*
+ * There is explicitly no include guard here because this file is expected to
+ * be included multiple times in order to define the syscall macros via
+ * __SYSCALL.
+ */
 
 /*
  * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
@@ -20,7 +23,7 @@
  * caller.  We don't currently do anything with the address range, that's just
  * in there for forwards compatibility.
  */
+#ifndef __NR_riscv_flush_icache
 #define __NR_riscv_flush_icache (__NR_arch_specific_syscall + 15)
-__SYSCALL(__NR_riscv_flush_icache, sys_riscv_flush_icache)
-
 #endif
+__SYSCALL(__NR_riscv_flush_icache, sys_riscv_flush_icache)
-- 
2.16.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
@ 2018-08-10  8:38   ` Christoph Hellwig
  2018-08-10 18:27     ` Palmer Dabbelt
  2018-08-10 14:07   ` Guenter Roeck
  2018-08-14 13:39   ` Christoph Hellwig
  2 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2018-08-10  8:38 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, linux, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, tklauser, linux-riscv, dan.carpenter

On Thu, Aug 09, 2018 at 03:19:51PM -0700, Palmer Dabbelt wrote:
> This would be necessary to make non-SMP builds work, but there is
> another error in the implementation of our syscall linkage that actually
> just causes sys_riscv_flush_icache to never build.  I've build tested
> this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
> normal.

Would't it make sense to use COND_SYSCALL to stub out the syscall
for !SMP builds?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
  2018-08-10  8:38   ` Christoph Hellwig
@ 2018-08-10 14:07   ` Guenter Roeck
  2018-08-14 13:39   ` Christoph Hellwig
  2 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2018-08-10 14:07 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, aou, tklauser, Arnd Bergmann, Andrew Waterman,
	linux, dan.carpenter, linux-riscv, linux-kernel

On Thu, Aug 09, 2018 at 03:19:51PM -0700, Palmer Dabbelt wrote:
> This would be necessary to make non-SMP builds work, but there is
> another error in the implementation of our syscall linkage that actually
> just causes sys_riscv_flush_icache to never build.  I've build tested
> this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
> normal.
> 
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Guenter Roeck <linux@roeck-us.net>
> In-Reply-To: <20180809055830.GA17533@infradead.org>
> In-Reply-To: <20180809132612.GA31058@roeck-us.net>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  arch/riscv/include/asm/vdso.h |  2 --
>  arch/riscv/kernel/sys_riscv.c | 12 ++++++++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/vdso.h b/arch/riscv/include/asm/vdso.h
> index 541544d64c33..ec6180a4b55d 100644
> --- a/arch/riscv/include/asm/vdso.h
> +++ b/arch/riscv/include/asm/vdso.h
> @@ -38,8 +38,6 @@ struct vdso_data {
>  	(void __user *)((unsigned long)(base) + __vdso_##name);			\
>  })
>  
> -#ifdef CONFIG_SMP
>  asmlinkage long sys_riscv_flush_icache(uintptr_t, uintptr_t, uintptr_t);
> -#endif
>  
>  #endif /* _ASM_RISCV_VDSO_H */
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index f7181ed8aafc..568026ccf6e8 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -48,7 +48,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>  }
>  #endif /* !CONFIG_64BIT */
>  
> -#ifdef CONFIG_SMP
>  /*
>   * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
>   * having a direct 'fence.i' instruction available to userspace (which we
> @@ -66,15 +65,24 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>  SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
>  	uintptr_t, flags)
>  {
> +#ifdef CONFIG_SMP
>  	struct mm_struct *mm = current->mm;
>  	bool local = (flags & SYS_RISCV_FLUSH_ICACHE_LOCAL) != 0;
> +#endif
>  
>  	/* Check the reserved flags. */
>  	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
>  		return -EINVAL;
>  
> +	/*
> +	 * Without CONFIG_SMP flush_icache_mm is a just a flush_icache_all(),
> +	 * which generates unused variable warnings all over this function.
> +	 */
> +#ifdef CONFIG_SMP
>  	flush_icache_mm(mm, local);
> +#else
> +	flush_icache_all();
> +#endif
>  
>  	return 0;
>  }
> -#endif
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h
  2018-08-09 22:19 ` [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h Palmer Dabbelt
@ 2018-08-10 14:07   ` Guenter Roeck
  2018-08-14 13:40   ` Christoph Hellwig
  1 sibling, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2018-08-10 14:07 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, aou, tklauser, Arnd Bergmann, Andrew Waterman,
	linux, dan.carpenter, linux-riscv, linux-kernel, Marcus Comstedt

On Thu, Aug 09, 2018 at 03:19:52PM -0700, Palmer Dabbelt wrote:
> This file is expected to be included multiple times in the same file in
> order to allow the __SYSCALL macro to generate system call tables.  With
> a global include guard we end up missing __NR_riscv_flush_icache in the
> syscall table, which results in icache flushes that escape the vDSO call
> to not actually do anything.
> 
> The fix is to move to per-#define include guards, which allows the
> system call tables to actually be populated.  Thanks to Macrus Comstedt
> for finding and fixing the bug!
> 
> I also went ahead and fixed the SPDX header to use a //-style comment,
> which I've been told is the canonical way to do it.
> 
> Cc: Marcus Comstedt <marcus@mc.pp.se>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  arch/riscv/include/asm/unistd.h        |  5 +++++
>  arch/riscv/include/uapi/asm/syscalls.h | 15 +++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
> index 080fb28061de..0caea01d5cca 100644
> --- a/arch/riscv/include/asm/unistd.h
> +++ b/arch/riscv/include/asm/unistd.h
> @@ -11,6 +11,11 @@
>   *   GNU General Public License for more details.
>   */
>  
> +/*
> + * There is explicitly no include guard here because this file is expected to
> + * be included multiple times.  See uapi/asm/syscalls.h for more info.
> + */
> +
>  #define __ARCH_WANT_SYS_CLONE
>  #include <uapi/asm/unistd.h>
>  #include <uapi/asm/syscalls.h>
> diff --git a/arch/riscv/include/uapi/asm/syscalls.h b/arch/riscv/include/uapi/asm/syscalls.h
> index 818655b0d535..690beb002d1d 100644
> --- a/arch/riscv/include/uapi/asm/syscalls.h
> +++ b/arch/riscv/include/uapi/asm/syscalls.h
> @@ -1,10 +1,13 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> +// SPDX-License-Identifier: GPL-2.0
>  /*
> - * Copyright (C) 2017 SiFive
> + * Copyright (C) 2017-2018 SiFive
>   */
>  
> -#ifndef _ASM__UAPI__SYSCALLS_H
> -#define _ASM__UAPI__SYSCALLS_H
> +/*
> + * There is explicitly no include guard here because this file is expected to
> + * be included multiple times in order to define the syscall macros via
> + * __SYSCALL.
> + */
>  
>  /*
>   * Allows the instruction cache to be flushed from userspace.  Despite RISC-V
> @@ -20,7 +23,7 @@
>   * caller.  We don't currently do anything with the address range, that's just
>   * in there for forwards compatibility.
>   */
> +#ifndef __NR_riscv_flush_icache
>  #define __NR_riscv_flush_icache (__NR_arch_specific_syscall + 15)
> -__SYSCALL(__NR_riscv_flush_icache, sys_riscv_flush_icache)
> -
>  #endif
> +__SYSCALL(__NR_riscv_flush_icache, sys_riscv_flush_icache)
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-10  8:38   ` Christoph Hellwig
@ 2018-08-10 18:27     ` Palmer Dabbelt
  2018-08-10 18:47       ` Guenter Roeck
  2018-08-14 13:35       ` Christoph Hellwig
  0 siblings, 2 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-08-10 18:27 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: aou, Andrew Waterman, Arnd Bergmann, linux-kernel, linux,
	Christoph Hellwig, dan.carpenter, tklauser, linux-riscv, linux

On Fri, 10 Aug 2018 01:38:04 PDT (-0700), Christoph Hellwig wrote:
> On Thu, Aug 09, 2018 at 03:19:51PM -0700, Palmer Dabbelt wrote:
>> This would be necessary to make non-SMP builds work, but there is
>> another error in the implementation of our syscall linkage that actually
>> just causes sys_riscv_flush_icache to never build.  I've build tested
>> this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
>> normal.
>
> Would't it make sense to use COND_SYSCALL to stub out the syscall
> for !SMP builds?

I'm not sure.  We can implement the syscall fine in !SMP, it's just that the 
vDSO is expected to always eat these calls because in non-SMP mode you can do a 
global fence.i by just doing a local fence.i (there's only one hart).

The original rationale behind not having the syscall in non-SMP mode was to 
limit the user ABI, but on looking again that seems like it's just a bit of 
extra complexity that doesn't help anything.  It's already been demonstrated 
that nothing is checking the error because it's been silently slipping past 
userspace for six months, so the extra complexity seems like it'll just cause 
someone else to have to chase the bug in the future.

But I'm really OK either way.  Is there a precedent for what to do here?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-10 18:27     ` Palmer Dabbelt
@ 2018-08-10 18:47       ` Guenter Roeck
  2018-08-10 20:55         ` Palmer Dabbelt
  2018-08-14 13:35       ` Christoph Hellwig
  1 sibling, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2018-08-10 18:47 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, dan.carpenter, tklauser, linux-riscv

On Fri, Aug 10, 2018 at 11:27:37AM -0700, Palmer Dabbelt wrote:
> On Fri, 10 Aug 2018 01:38:04 PDT (-0700), Christoph Hellwig wrote:
> >On Thu, Aug 09, 2018 at 03:19:51PM -0700, Palmer Dabbelt wrote:
> >>This would be necessary to make non-SMP builds work, but there is
> >>another error in the implementation of our syscall linkage that actually
> >>just causes sys_riscv_flush_icache to never build.  I've build tested
> >>this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
> >>normal.
> >
> >Would't it make sense to use COND_SYSCALL to stub out the syscall
> >for !SMP builds?
> 
> I'm not sure.  We can implement the syscall fine in !SMP, it's just that the
> vDSO is expected to always eat these calls because in non-SMP mode you can
> do a global fence.i by just doing a local fence.i (there's only one hart).
> 
> The original rationale behind not having the syscall in non-SMP mode was to
> limit the user ABI, but on looking again that seems like it's just a bit of
> extra complexity that doesn't help anything.  It's already been demonstrated

Doesn't this mean that some userspace code will only run if the kernel was
compiled for SMP ? I always thought that was unacceptable.

Guenter

> that nothing is checking the error because it's been silently slipping past
> userspace for six months, so the extra complexity seems like it'll just
> cause someone else to have to chase the bug in the future.
> 
> But I'm really OK either way.  Is there a precedent for what to do here?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-10 18:47       ` Guenter Roeck
@ 2018-08-10 20:55         ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-08-10 20:55 UTC (permalink / raw)
  To: linux
  Cc: Christoph Hellwig, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, dan.carpenter, tklauser, linux-riscv

On Fri, 10 Aug 2018 11:47:15 PDT (-0700), linux@roeck-us.net wrote:
> On Fri, Aug 10, 2018 at 11:27:37AM -0700, Palmer Dabbelt wrote:
>> On Fri, 10 Aug 2018 01:38:04 PDT (-0700), Christoph Hellwig wrote:
>> >On Thu, Aug 09, 2018 at 03:19:51PM -0700, Palmer Dabbelt wrote:
>> >>This would be necessary to make non-SMP builds work, but there is
>> >>another error in the implementation of our syscall linkage that actually
>> >>just causes sys_riscv_flush_icache to never build.  I've build tested
>> >>this on allnoconfig and allnoconfig+SMP=y, as well as defconfig like
>> >>normal.
>> >
>> >Would't it make sense to use COND_SYSCALL to stub out the syscall
>> >for !SMP builds?
>>
>> I'm not sure.  We can implement the syscall fine in !SMP, it's just that the
>> vDSO is expected to always eat these calls because in non-SMP mode you can
>> do a global fence.i by just doing a local fence.i (there's only one hart).
>>
>> The original rationale behind not having the syscall in non-SMP mode was to
>> limit the user ABI, but on looking again that seems like it's just a bit of
>> extra complexity that doesn't help anything.  It's already been demonstrated
>
> Doesn't this mean that some userspace code will only run if the kernel was
> compiled for SMP ? I always thought that was unacceptable.

Well, the officially sanctioned way to obtain this functionality is via a vDSO 
call.  On non-SMP systems it will never make the system call.  As a result we 
thought we'd keep it out of the ABI, but after looking again it seems yucky to 
do so.  Here's the vDSO entry, for reference:

    ENTRY(__vdso_flush_icache)
            .cfi_startproc
    #ifdef CONFIG_SMP
            li a7, __NR_riscv_flush_icache
            ecall
    #else
            fence.i
            li a0, 0
    #endif
            ret
            .cfi_endproc
    ENDPROC(__vdso_flush_icache)

Note that glibc has a fallback to make the system call if it can't find the 
vDSO entry, but then doesn't have a secondary fallback to emit a local fence.i 
if the system call doesn't exist.  It seems easier to fix the kernel to always 
provide the syscall and just call it a bug.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-10 18:27     ` Palmer Dabbelt
  2018-08-10 18:47       ` Guenter Roeck
@ 2018-08-14 13:35       ` Christoph Hellwig
  1 sibling, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-08-14 13:35 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, linux, tklauser, linux-riscv, dan.carpenter

On Fri, Aug 10, 2018 at 11:27:37AM -0700, Palmer Dabbelt wrote:
> I'm not sure.  We can implement the syscall fine in !SMP, it's just that the
> vDSO is expected to always eat these calls because in non-SMP mode you can
> do a global fence.i by just doing a local fence.i (there's only one hart).
> 
> The original rationale behind not having the syscall in non-SMP mode was to
> limit the user ABI, but on looking again that seems like it's just a bit of
> extra complexity that doesn't help anything.  It's already been demonstrated
> that nothing is checking the error because it's been silently slipping past
> userspace for six months, so the extra complexity seems like it'll just
> cause someone else to have to chase the bug in the future.
> 
> But I'm really OK either way.  Is there a precedent for what to do here?

I don't know of any.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
  2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
  2018-08-10  8:38   ` Christoph Hellwig
  2018-08-10 14:07   ` Guenter Roeck
@ 2018-08-14 13:39   ` Christoph Hellwig
  2 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-08-14 13:39 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, linux, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, tklauser, linux-riscv, dan.carpenter

>  SYSCALL_DEFINE3(riscv_flush_icache, uintptr_t, start, uintptr_t, end,
>  	uintptr_t, flags)
>  {
> +#ifdef CONFIG_SMP
>  	struct mm_struct *mm = current->mm;
>  	bool local = (flags & SYS_RISCV_FLUSH_ICACHE_LOCAL) != 0;
> +#endif
>  
>  	/* Check the reserved flags. */
>  	if (unlikely(flags & ~SYS_RISCV_FLUSH_ICACHE_ALL))
>  		return -EINVAL;
>  
> +	/*
> +	 * Without CONFIG_SMP flush_icache_mm is a just a flush_icache_all(),
> +	 * which generates unused variable warnings all over this function.
> +	 */
> +#ifdef CONFIG_SMP
>  	flush_icache_mm(mm, local);
> +#else
> +	flush_icache_all();
> +#endif

Eeek.

Something like an unconditional:

	flush_icache_mm(current->mm, flags & SYS_RISCV_FLUSH_ICACHE_LOCAL);

should solve those issues.

Also in the longer run we should turn the !SMP flush_icache_mm stub
into an inline function to solve this problem for all potential
callers.  Excepte that flush_icache_mm happens to be a RISC-V specific
API without any other callers.  So for now I think the above is what
I'd do, but this area has a lot of room for cleanup.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h
  2018-08-09 22:19 ` [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h Palmer Dabbelt
  2018-08-10 14:07   ` Guenter Roeck
@ 2018-08-14 13:40   ` Christoph Hellwig
  1 sibling, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-08-14 13:40 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Christoph Hellwig, linux, aou, Andrew Waterman, Arnd Bergmann,
	linux-kernel, linux, Marcus Comstedt, tklauser, linux-riscv,
	dan.carpenter

> index 818655b0d535..690beb002d1d 100644
> --- a/arch/riscv/include/uapi/asm/syscalls.h
> +++ b/arch/riscv/include/uapi/asm/syscalls.h
> @@ -1,10 +1,13 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> +// SPDX-License-Identifier: GPL-2.0

/* ... */  is the right style SPDX tag for headers, so please keep it
as-is.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-09 22:19 RISC-V: Don't use a global include guard for uapi/asm/syscalls Palmer Dabbelt
2018-08-09 22:19 ` [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n Palmer Dabbelt
2018-08-10  8:38   ` Christoph Hellwig
2018-08-10 18:27     ` Palmer Dabbelt
2018-08-10 18:47       ` Guenter Roeck
2018-08-10 20:55         ` Palmer Dabbelt
2018-08-14 13:35       ` Christoph Hellwig
2018-08-10 14:07   ` Guenter Roeck
2018-08-14 13:39   ` Christoph Hellwig
2018-08-09 22:19 ` [PATCH v3 2/2] RISC-V: Don't use a global include guard for uapi/asm/syscalls.h Palmer Dabbelt
2018-08-10 14:07   ` Guenter Roeck
2018-08-14 13:40   ` Christoph Hellwig

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox