Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681158 > unrolled thread

RISC-V Linux Port v4

Back to article view | Back to linux.kernel

Contents

  RISC-V Linux Port v4 Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-04 22:00 +0200
    Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code Karsten Merker <merker@debian.org> - 2017-07-04 23:20 +0200
      Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code Thomas Gleixner <tglx@linutronix.de> - 2017-07-05 08:50 +0200
    Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-05 00:00 +0200
      Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-07 00:40 +0200
        Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-07 15:10 +0200
          Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:50 +0200
    Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-05 10:50 +0200
      Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-06 06:20 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-06 09:30 +0200
      Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-07 03:10 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-07 04:20 +0200
          Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:40 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-07 10:10 +0200
          Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:40 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API James Hogan <james.hogan@imgtec.com> - 2017-07-05 12:30 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-06 04:10 +0200
      Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-06 11:00 +0200
        Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-06 17:40 +0200
          Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-06 17:50 +0200
            Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-11 15:30 +0200
              Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-11 16:00 +0200
                Re: [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-11 19:30 +0200
              Re: [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-11 19:10 +0200
    Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-06 05:40 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API Dave P Martin <Dave.Martin@arm.com> - 2017-07-06 17:40 +0200
    Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-07 15:20 +0200
      Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1685030 — Re: [PATCH 8/9] RISC-V: User-facing API

On Mon, Jul 10, 2017 at 01:00:29PM -0700, Palmer Dabbelt wrote:
> On Thu, 06 Jul 2017 08:45:13 PDT (-0700), will.deacon@arm.com wrote:
> > On Thu, Jul 06, 2017 at 08:34:27AM -0700, Christoph Hellwig wrote:
> >> On Thu, Jul 06, 2017 at 09:55:03AM +0100, Will Deacon wrote:
> >> > Agreed on the indirection; it feels like this is something that should be in
> >> > the vDSO, which could use the cmpxchg instruction if it's available, or
> >> > otherwise just uses plain loads and stores.
> 
> These are already in the vDSO, and use the corresponding atomic instructions on
> systems with the A extension.  The vDSO routines call the system calls in non-A
> systems.  As far as I can tell that's necessary to preserve atomicity, which we
> currently do by disabling scheduling.  If there's a way to do this without
> entering the kernel then I'd be happy to support it, but I'm not sure how we
> could maintain atomicity using only regular loads and stores.

Take a look at the ARM code I mentioned. You can do away with the syscall if
you notice that you preempt a thread inside the critical section of the
vDSO, and, in that case you resume execution at a known "restart" address.

> >> Even that seems like a lot of indirection for something that is in
> >> the critical fast path for synchronization.  I really can't understand
> >> how a new ISA / ABI could even come up with an idea as stupid as making
> >> essential synchronization primitives optional.
> >
> > No disagreement there!
> 
> The default set of multilibs on Linux are:
> 
>  * rv32imac: 32-bit; Multiply, Atomic, and Compressed extensions
>  * rv32imafdc: like above, but with single+double float
>  * rv64imac: 64-bit, Multiply, Atomic and Compressed
>  * rv64imafdc: like above, but with single+double float
> 
> all of which support the A extension.  We certainly don't plan on building any
> systems that support Linux without the A extension at SiFive, so I'm fine
> removing the system call -- this was originally added by a user, so there was
> at least enough interest for someone to add the system call.
> 
> We've found people are retrofitting other cores to run RISC-V, and I could
> certainly imagine an older design that lacks a beefy enough memory system to
> support our atomics (which are LR/SC based) being a design that might arise.
> There's a lot of systems where people don't seem to care that much about the
> performance and just want something to work -- if they're on such a tiny system
> they can't implement the A extension then they're probably not going to be
> doing a lot of atomics anyway, so maybe it doesn't matter if atomics are slow.
> As the cost for supporting these A-less systems seems fairly small, it seemed
> like the right thing to do -- one of the points of making RISC-V have many
> optional extensions was to let people pick the ones they view as important.
> Since I don't know the performance constraints of their systems or the cost of
> implementing the A extension in their design, I'm not really qualified to tell
> them a cmpxchg syscall is a bad idea.

The problem is that by supporting these hypothetical designs that can't do
atomics, you hurt sensible designs that *can* do the atomics because you
force them to take an additional indirection that could otherwise be
avoided.

Will

[toc] | [prev] | [next] | [standalone]

#1685042 — Re: [PATCH 8/9] RISC-V: User-facing API

On Tue, Jul 11, 2017 at 02:22:15PM +0100, Will Deacon wrote:
> The problem is that by supporting these hypothetical designs that can't do
> atomics, you hurt sensible designs that *can* do the atomics because you
> force them to take an additional indirection that could otherwise be
> avoided.

Agreed.  But the new patchset seems to remove it already, so I guess
we're fine on the kernel side.  Now we just need to make sure the
glibc API doesn't use any indirections.

Note that it might make sense to emit these for very low end nommu
designs.  Maybe even running Linux, but in that case they'll just need
a special non-standard ABI for very limited use cases.

[toc] | [prev] | [next] | [standalone]

#1685240 — Re: [PATCH 8/9] RISC-V: User-facing API

On Tue, 11 Jul 2017 06:55:28 PDT (-0700), hch@infradead.org wrote:
> On Tue, Jul 11, 2017 at 02:22:15PM +0100, Will Deacon wrote:
>> The problem is that by supporting these hypothetical designs that can't do
>> atomics, you hurt sensible designs that *can* do the atomics because you
>> force them to take an additional indirection that could otherwise be
>> avoided.
>
> Agreed.  But the new patchset seems to remove it already, so I guess
> we're fine on the kernel side.  Now we just need to make sure the
> glibc API doesn't use any indirections.
>
> Note that it might make sense to emit these for very low end nommu
> designs.  Maybe even running Linux, but in that case they'll just need
> a special non-standard ABI for very limited use cases.

glibc has never used these calls on machines with the A extension.  They're
only used in one specific header file to emulate cmpxchg, and they're guarded
by something like "#ifdef riscv_atomic".  Here's the glibc code (from a
slightly older version, glibc is also in submission so everything's a bit of a
mess there too) for reference

  /* If the A (atomic) extension is not present, we need help from the
     kernel to do atomic accesses.  Linux provides two system calls for
     this purpose.  RISCV_ATOMIC_CMPXCHG will perform an atomic compare
     and exchange operation for a 32-bit value.  RISCV_ATOMIC_CMPXCHG64
     will do the same for a 64-bit value. */

  #include <sys/syscall.h>
  #include <sysdep.h>

  #define __HAVE_64B_ATOMICS (__riscv_xlen >= 64)
  #define USE_ATOMIC_COMPILER_BUILTINS 0

  #define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
    (abort (), (__typeof (*mem)) 0)

  #define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
    (abort (), (__typeof (*mem)) 0)

  /* The only basic operation needed is compare and exchange.  */
  #define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
    ({									      \
      INTERNAL_SYSCALL_DECL (__err);					      \
      (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
  		      RISCV_ATOMIC_CMPXCHG, mem, oldval, newval);	      \
    })

  #define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
    ({									      \
      INTERNAL_SYSCALL_DECL (__err);					      \
      (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
  		      RISCV_ATOMIC_CMPXCHG64, mem, oldval, newval);	      \
    })

We originally had these as a special Kconfig option, but then it was pointed
out that user binaries built on non-A systems wouldn't run on A systems.  That
seemed like a bad idea, so we just enabled it everywhere.

I think we should just table this discussion for now: we can always add the
system calls back in if people build non-A Linux systems.  We'll mark our glibc
port as requiring the A extension and delete the dead code there so nothing
knows about the syscalls.

Thanks!

[toc] | [prev] | [next] | [standalone]

#1685215 — Re: [PATCH 8/9] RISC-V: User-facing API

On Tue, 11 Jul 2017 06:22:15 PDT (-0700), will.deacon@arm.com wrote:
> On Mon, Jul 10, 2017 at 01:00:29PM -0700, Palmer Dabbelt wrote:
>> On Thu, 06 Jul 2017 08:45:13 PDT (-0700), will.deacon@arm.com wrote:
>> > On Thu, Jul 06, 2017 at 08:34:27AM -0700, Christoph Hellwig wrote:
>> >> On Thu, Jul 06, 2017 at 09:55:03AM +0100, Will Deacon wrote:
>> >> > Agreed on the indirection; it feels like this is something that should be in
>> >> > the vDSO, which could use the cmpxchg instruction if it's available, or
>> >> > otherwise just uses plain loads and stores.
>>
>> These are already in the vDSO, and use the corresponding atomic instructions on
>> systems with the A extension.  The vDSO routines call the system calls in non-A
>> systems.  As far as I can tell that's necessary to preserve atomicity, which we
>> currently do by disabling scheduling.  If there's a way to do this without
>> entering the kernel then I'd be happy to support it, but I'm not sure how we
>> could maintain atomicity using only regular loads and stores.
>
> Take a look at the ARM code I mentioned. You can do away with the syscall if
> you notice that you preempt a thread inside the critical section of the
> vDSO, and, in that case you resume execution at a known "restart" address.
>
>> >> Even that seems like a lot of indirection for something that is in
>> >> the critical fast path for synchronization.  I really can't understand
>> >> how a new ISA / ABI could even come up with an idea as stupid as making
>> >> essential synchronization primitives optional.
>> >
>> > No disagreement there!
>>
>> The default set of multilibs on Linux are:
>>
>>  * rv32imac: 32-bit; Multiply, Atomic, and Compressed extensions
>>  * rv32imafdc: like above, but with single+double float
>>  * rv64imac: 64-bit, Multiply, Atomic and Compressed
>>  * rv64imafdc: like above, but with single+double float
>>
>> all of which support the A extension.  We certainly don't plan on building any
>> systems that support Linux without the A extension at SiFive, so I'm fine
>> removing the system call -- this was originally added by a user, so there was
>> at least enough interest for someone to add the system call.
>>
>> We've found people are retrofitting other cores to run RISC-V, and I could
>> certainly imagine an older design that lacks a beefy enough memory system to
>> support our atomics (which are LR/SC based) being a design that might arise.
>> There's a lot of systems where people don't seem to care that much about the
>> performance and just want something to work -- if they're on such a tiny system
>> they can't implement the A extension then they're probably not going to be
>> doing a lot of atomics anyway, so maybe it doesn't matter if atomics are slow.
>> As the cost for supporting these A-less systems seems fairly small, it seemed
>> like the right thing to do -- one of the points of making RISC-V have many
>> optional extensions was to let people pick the ones they view as important.
>> Since I don't know the performance constraints of their systems or the cost of
>> implementing the A extension in their design, I'm not really qualified to tell
>> them a cmpxchg syscall is a bad idea.
>
> The problem is that by supporting these hypothetical designs that can't do
> atomics, you hurt sensible designs that *can* do the atomics because you
> force them to take an additional indirection that could otherwise be
> avoided.

I just went ahead and removed the system calls from the port -- they're not
going to get called on any systems SiFive is building, so if someone complains
then we'll just sort it out later.

[toc] | [prev] | [next] | [standalone]

#1682030 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
[...]
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> new file mode 100644
> index 000000000000..81025c056412
> --- /dev/null
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright (C) 2014 Regents of the University of California
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#ifndef _ASM_RISCV_CMPXCHG_H
> +#define _ASM_RISCV_CMPXCHG_H
> +
> +#include <linux/bug.h>
> +
> +#ifdef CONFIG_ISA_A
> +
> +#include <asm/barrier.h>
> +
> +#define __xchg(new, ptr, size, asm_or)				\
> +({								\
> +	__typeof__(ptr) __ptr = (ptr);				\
> +	__typeof__(new) __new = (new);				\
> +	__typeof__(*(ptr)) __ret;				\
> +	switch (size) {						\
> +	case 4:							\
> +		__asm__ __volatile__ (				\
> +			"amoswap.w" #asm_or " %0, %2, %1"	\
> +			: "=r" (__ret), "+A" (*__ptr)		\
> +			: "r" (__new));				\

It seems that you miss the "memmory" clobber here, so as for cmpxchg(),
did you do this on purpose? AFAIK, without this clobber, compilers are
within their right to reorder operations preceding and following this
operation, which makes it unordered.

> +		break;						\
> +	case 8:							\
> +		__asm__ __volatile__ (				\
> +			"amoswap.d" #asm_or " %0, %2, %1"	\
> +			: "=r" (__ret), "+A" (*__ptr)		\
> +			: "r" (__new));				\
> +		break;						\
> +	default:						\
> +		BUILD_BUG();					\
> +	}							\
> +	__ret;							\
> +})
> +
> +#define xchg(ptr, x)    (__xchg((x), (ptr), sizeof(*(ptr)), .aqrl))
> +
> +#define xchg32(ptr, x)				\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 4);	\
> +	xchg((ptr), (x));			\
> +})
> +
> +#define xchg64(ptr, x)				\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 8);	\
> +	xchg((ptr), (x));			\
> +})
> +
> +/*
> + * Atomic compare and exchange.  Compare OLD with MEM, if identical,
> + * store NEW in MEM.  Return the initial value in MEM.  Success is
> + * indicated by comparing RETURN with OLD.
> + */
> +#define __cmpxchg(ptr, old, new, size, lrb, scb)			\
> +({									\
> +	__typeof__(ptr) __ptr = (ptr);					\
> +	__typeof__(old) __old = (old);					\
> +	__typeof__(new) __new = (new);					\

Better write those two lines as:

	__typeof__(*(ptr)) __old = (old);					\
	__typeof__(*(ptr)) __new = (new);					\

? I'm thinking the case where @old and @new are int and ptr is "long *",
could the asm below do the implicitly converting right, i.e. keep the
sign bit?

Regards,
Boqun

> +	__typeof__(*(ptr)) __ret;					\
> +	register unsigned int __rc;					\
> +	switch (size) {							\
> +	case 4:								\
> +		__asm__ __volatile__ (					\
> +		"0:"							\
> +			"lr.w" #scb " %0, %2\n"				\
> +			"bne         %0, %z3, 1f\n"			\
> +			"sc.w" #lrb " %1, %z4, %2\n"			\
> +			"bnez        %1, 0b\n"				\
> +		"1:"							\
> +			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
> +			: "rJ" (__old), "rJ" (__new));			\
> +		break;							\
> +	case 8:								\
> +		__asm__ __volatile__ (					\
> +		"0:"							\
> +			"lr.d" #scb " %0, %2\n"				\
> +			"bne         %0, %z3, 1f\n"			\
> +			"sc.d" #lrb " %1, %z4, %2\n"			\
> +			"bnez        %1, 0b\n"				\
> +		"1:"							\
> +			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
> +			: "rJ" (__old), "rJ" (__new));			\
> +		break;							\
> +	default:							\
> +		BUILD_BUG();						\
> +	}								\
> +	__ret;								\
> +})
> +
> +#define cmpxchg(ptr, o, n) \
> +	(__cmpxchg((ptr), (o), (n), sizeof(*(ptr)), .aqrl, .aqrl))
> +
> +#define cmpxchg_local(ptr, o, n) \
> +	(__cmpxchg((ptr), (o), (n), sizeof(*(ptr)), , ))
> +
> +#define cmpxchg32(ptr, o, n)			\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 4);	\
> +	cmpxchg((ptr), (o), (n));		\
> +})
> +
> +#define cmpxchg32_local(ptr, o, n)		\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 4);	\
> +	cmpxchg_local((ptr), (o), (n));		\
> +})
> +
> +#define cmpxchg64(ptr, o, n)			\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 8);	\
> +	cmpxchg((ptr), (o), (n));		\
> +})
> +
> +#define cmpxchg64_local(ptr, o, n)		\
> +({						\
> +	BUILD_BUG_ON(sizeof(*(ptr)) != 8);	\
> +	cmpxchg_local((ptr), (o), (n));		\
> +})
> +
> +#else /* !CONFIG_ISA_A */
> +
> +#include <asm-generic/cmpxchg.h>
> +
> +#endif /* CONFIG_ISA_A */
> +

[...]

[toc] | [prev] | [next] | [standalone]

#1682518 — Re: [PATCH 8/9] RISC-V: User-facing API

On Wed, Jul 05, 2017 at 09:49:36AM -0700, Palmer Dabbelt wrote:
> On Mon, 03 Jul 2017 16:06:39 PDT (-0700), james.hogan@imgtec.com wrote:
> > On Thu, Jun 29, 2017 at 02:42:38PM -0700, Palmer Dabbelt wrote:
> >> On Wed, 28 Jun 2017 15:42:37 PDT (-0700), james.hogan@imgtec.com wrote:
> >> > On Wed, Jun 28, 2017 at 11:55:37AM -0700, Palmer Dabbelt wrote:
> >> >> diff --git a/arch/riscv/include/uapi/asm/ucontext.h b/arch/riscv/include/uapi/asm/ucontext.h
> >> >> new file mode 100644
> >> >> index 000000000000..52eff9febcfd
> >> >> --- /dev/null
> >> >> +++ b/arch/riscv/include/uapi/asm/ucontext.h
> >> > ...
> >> >> +struct ucontext {
> >> >> +	unsigned long	  uc_flags;
> >> >> +	struct ucontext	 *uc_link;
> >> >> +	stack_t		  uc_stack;
> >> >> +	sigset_t	  uc_sigmask;
> >> >> +	/* glibc uses a 1024-bit sigset_t */
> >> >> +	__u8		  __unused[1024 / 8 - sizeof(sigset_t)];
> >> >> +	/* last for future expansion */
> >> >> +	struct sigcontext uc_mcontext;
> >> >> +};
> >> >
> >> > Any particular reason not to use the asm-generic ucontext?
> >>
> >> In the generic ucontext, 'uc_sigmask' is at the end of the structure so it can
> >> be expanded.  Since we want our mcontext to be expandable as well, we
> >> pre-allocate some expandable space for sigmask and then put mcontext at the
> >> end.
> >>
> >> We stole this idea from arm64.
> >
> > Curious. __unused seems like overkill to be honest given that expanding
> > the number of signals up to 128 causes other issues (as discovered on
> > MIPS e.g. the waitpid() status, with stopsig not fitting below the exit
> > code (shift 8) and core dump flag (bit 7)), but perhaps it could be
> > carefully expanded by splitting the stopsig field.
> 
> Sorry, I don't understand the intricacies of this in the slightest.  In general
> we try to avoid surprises in software land in RISC-V, so whenever we do
> something we go look at the most popular architectures (Intel and ARM) and try
> to ensure we don't paint ourselves into any corners that they didn't.

I think Catalin was concerned that putting uc_sigmask at the end breaks
extensibility of sigcontext.

[Catalin, please comment if I'm misquoting you ;) ]


Generic ucontext seems broken in any case, when kernel/user sigset_t
sizes differ:

sigset_t oldmask, newmask;

void handler(int n, siginfo_t *si, void *uc_)
{
	ucontext_t *uc = uc_;

	oldmask = uc->uc_sigmask; uc->uc_sigmask = newmask;
}

With generic ucontext, this can overrun and corrupt memory if the user/
kernel sigset_t sizes differ.  The only fix is to reserve space in
ucontext for the larger of the two sigset sizes, which generic ucontext
does not do.

There's also the problem you comment on where only 7 bits of signal
number are available in wait() status values: with 0x7f being magic and
0 not a valid signal number, that probably allows up to 126 signals.
An arch could possibly have its own definitions to get beyond this, but
it's all done by magic numbers and open-coding in core code today.

i.e., the "extensibility" in generic ucontext may be bogus.


So, you can commit to a sane maximum number of signals (say, 64) in
your ABI, but this means that your libc sigset_t size probably needs to
match and you can never grow beyond that without an ABI break.

Or you can reserve enough space in ucontext for the userspace sigset_t.
Using generic signal.h and ucontext.h effectively commits you to max 64
signals AFAICT.  The extra space may be permanently wasted, but that's
preferable to memory corruption.


(Note, I don't know myself where the "1024" comes from.  Are there any
POSIXish systems implementing anywhere near that number of signals?  Is
there a real usecase for it?  Maybe it's just overzealous
futureproofing?)

> > Looks harmless here I suppose so I defer to others. If it is the
> > preferred approach does it make sense to make it the "default" for new
> > architectures at some point?
> 
> Again, this isn't really my thing, but we chose this because we thought it was
> the sane way to do it.  Unless we're doing something silly, I don't see why it
> wouldn't be a reasonable default.  This is predicated on having expandable
> architectural state, otherwise putting sigmask at the end seems sane.

Note, the contents of sigcontext are nonportable but are nonetheless
ABI, and some userspace software does expect to be able to poke about in
there, modify the signal return state, etc.

Additionally the whole signal frame cannot safely exceed MINSIGSTKSZ in
size (which looks like 2K if you're relying on generic signal.h -- not
a big number when compared against upcoming vector architectures).

Going beyond MINSIGSTKSZ is a user ABI break, as is any non-probeable
change to the contents of struct sigcontext, including changes to its
size.


We are burned by this on arm64 with SVE: arm64 has its own MINSIGSTKSZ
(5K), which is not enough for the biggest possible SVE implemetations
(over 8K for the SVE state alone).  I have some proposals to mitigate
this [1], but a complete solution is not possible.

Without any flags, size or version field or some kind of extensible
list structure, you would likely have trouble extending your sigcontext
without ABI breaks.


The ucontext API is unfortunately fundamentally broken, especially with
regard to extensibility.  It's debatable whether the context argument
to sigaction handlers should have been ucontext_t, but we seem to be
stuck with it.  POSIX no longer attempts to specify most of the de
facto ucontext API behaviour, but it persists because there are things
that userspace can't do any other way.

In principle we could cook up a new signal interface, and a new arch
could avoid the current and legacy interfaces entirely.

For example, the signal context could be made properly extensible, and
a cookie could be registered for each handler so that libc can wrap
signal handlers without the need for runtime-generated trampolines.
Then a POSIX compatibilty interface could be implemented in libc.

Could be a tough sell though -- and there's a fair risk we'd come up
with something that is still broken.


Cheers
---Dave

[1] [RFC PATCH v2 0/6] Signal frame expansion support
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/501163.html

(Now merged, except for the AT_MINSIGSTKSZ auxv entry to report the
signal frame size -- since we won't need this until later and it could
benefit from more discussion and it would be good to build some
consensus around it if possible.  I plan to talk about this and
related topics at Plumbers.)

[toc] | [prev] | [next] | [standalone]

#1683179 — Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
[...]
> +/* These barries need to enforce ordering on both devices or memory. */

Very minor nit: s/barries/barriers/ (in several places)


Jonathan Neuschäfer

[toc] | [prev] | [next] | [standalone]

#1684629 — Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code

On Fri, 07 Jul 2017 06:16:07 PDT (-0700), j.neuschaefer@gmx.net wrote:
> On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
> [...]
>> +/* These barries need to enforce ordering on both devices or memory. */
>
> Very minor nit: s/barries/barriers/ (in several places)

I think this should do it

  https://github.com/riscv/riscv-linux/commit/b356e7a2223b5e21df424ea7e9900e5bf408762f

Thanks!

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web