Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
On Tue, 14 Aug 2018 06:39:23 PDT (-0700), Christoph Hellwig wrote: 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. Thanks, that's a lot cleaner. I missed this for the PR, but I'll submit a cleanup patch after RC1.
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
> 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.
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
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.
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
On Fri, 10 Aug 2018 11:47:15 PDT (-0700), li...@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.
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
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?
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
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?
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
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 > CC: Guenter Roeck > In-Reply-To: <20180809055830.ga17...@infradead.org> > In-Reply-To: <20180809132612.ga31...@roeck-us.net> > Signed-off-by: Palmer Dabbelt Tested-by: Guenter Roeck > --- > 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 >
Re: [PATCH v3 1/2] RISC-V: Define sys_riscv_flush_icache when SMP=n
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?